[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 <[EMAIL PROTECTED]>:
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

_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev