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

Hi, I'll cut away the questions I don't need to ask more about. 2008/5/29 Emmanuel Bernard <emmanuel(a)hibernate.org&gt;: > 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