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

[Emmanuel Bernard]

On  May 29, 2008, at 08:59, Sanne Grinovero wrote:

> 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.

Go ahead

>
>
>>> 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.

You can't do (d) really as one of the static method is used outside,  
you cannot do (c).
We could move static methods to a different class but (a) is easier  
for now. This is not like it would greatly enhance the software :)


>
>
>>> 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.

I am fine with trying to enhance but I will not give up of varargs and  
provided that a session must not be used in a multithreaded way. I am  
not sure we should pay the price of copying the array in this case.

>
>
>>> 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)

Is the load of the SharedReaderProvider class lazy even with the  
direct reference to it in ReaderProviderFactory? Otherwise that might  
trigger the exception for no good purpose.

>
>
>>> 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?

Perf

>
>
> Sanne