[hibernate-dev] Re: Hibernate Search: approval for changes, "commit conventions"?

[Sanne Grinovero]

Hi, I'll cut away the questions I don't need to ask more about.

2008/5/29 Emmanuel Bernard <emmanuel at hibernate.org>:
> Hi Sanne
>
> On  May 28, 2008, at 14:01, Sanne Grinovero wrote:
[...]
>> 1)is it ok in case of a "code revision" to have a commit of >100 files
>> for very trivial changes ( wrong formatting, spelling typos in comments,
>> unused import removals);
>> do I have to warn you before commits of this type?
>> should I leave them all as-is?
>
> If you do this massive work, you need to warn people in advance and do it in
> a day. It essentially means people will have hard time to sync up their
> current work while you do that.
> If you cna't then, do it by small batches.

Yes I understand that, but no code changes are involved just some comments,
imports and "final" changes so I hope not to cause difficult conflicts, if any.
How should I proceed? My changes are ready for commit, I just wait
for everybody saying it's ok, so I can update first. I've seen Hardy
committed this morning;
Also if my working this way goes in the way of other developers,
I'll just revert them all.

>> 5)static logger:
>> some classes (e.g. org.hibernate.search.engine.QueryLoader) "forget"
>> to declare the Logger as static, if this is intentional I would like to
>> know
>> more about it. May I fix them all if I find more?
>> Is it because of hot-redeploy? In this case I would need to remove
>> some statics;-)
>
> http://www.slf4j.org/faq.html#declared_static
> But for classes used a lot, I tend to keep the static modifier.

Nice I didn't now that; I changed the logger I used for configuration
parsing to non-static, looks much better.
Also I think DocumentBuilder could benefit from a non-static logger,
but it is (also) being used by one static method for a trace; I could
a) leave it static
b) have the method get it's own logger
c) remove the tracing
d) convert all using methods from static
I would prefer c), or d) but this may create a lot of changes.

>> A)defensive copy on public API (FindBugs):
>> FindBugs is complaining about
>> "May expose internal representation by incorporating reference to
>> mutable object"
>> in serveral places.
>> I suppose we can trust our own code, but for example in
>> FullTextQuery setProjection(String... fields)
>> it would be wise to make an own private copy of the data.
>> Too much paranoia? Also it would have some performance cost.
>
> What do you want to copy? Projection is essentially a copy already (unless
> you reference THIS but in this case you want the mutable object). Otherwise,
> the method parameters are immutable, so this is not a problem.

I guess this was not a very good example;) FindBugs is speaking about the Array,
a fool user may initialize the arguments as an array, give it to the
method, keeping
a reference to the array, to later (concurrently?) change the
references to other objects
or change positions, so having access to Searche's "internal state".
My questions is a bit more general, not specifically about this method but about
us ignoring this FindBugs warning, be aware of it; I suppose it would
be too much
paranoia to really change all the dangerous methods, just verifying it is
an issue you are aware of; rock solid libraries shouldn't allow it,
but it has performance
impact.

>> B) there's a STATIC field in SharedReaderProvider:
>> "static Field subReadersField"
>> it is looking quite suspect, is that really meant to be static?
>
> Yes it's on purpose, it's a pointer to a Field object, no need to pay the
> lookup more than once.
Ah sorry I should have looked better, I thought a Lucene Field, not reflection.
May I make it final, to enhance readability and to avoid null checks in code?
(initializing it with a static helper method)

>> C)Workspace contains many maps to needed objects,
>> all having the same key "DirectoryProvider"; also locks are
>> mantained in an array; Woudn't it be easier to have a single map,
>> returning a container object with all needed stuff;
>> This object could be passed to all other methods instead of the
>> dir.Provider and avoid many more gets on the maps.
>> also some locking code could go "local" in this container
>> class, I've some difficulty in tracking the lock sequences.
>
> Go ahead (open a JIRA issue), make sure it does no slow things down - it
> shouldn't from what I've seen)

You mean slow down of development time or Search's performance?

Sanne