8:01 p.m.
Hi Emmanuel,
(looking around the sources again) I am making several changes,
but I am feeling the urgency to ask your opinion about
"commit conventions"
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?
2)There are some "//TODO serialization" on classes that could implement
Serializable trivially, is it ok if I just add them ?
(I'll don't do it for cases I have doubts)
3)//TODO set a serial number
May I auto-generate a serialVersionUID for classes were missing,
or do you have some special policy about them I should be aware of?
4)final fields:
some constructor-initialized fields may need the final keyword, especially
wen producing thread-safe transfer objects; I've added several already,
I'm wondering now if it's ok to have this changes committed together
with 1) or if you would have preferred separated commits.
Also using "final" really helps me in better understanding the overall code.
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;-)
6)java.util.collections
there's a nice
List EMPTY_LIST = new ArrayList( 0 );
in QueryLoader; I would like to replace it with java.util.Collections.EMPTY_LIST
but this has another effect: UnsupportedOperationException if someone
tries to add
elements. I think this is a good thing, but I could be wrong.
Also currently someone could reread data in other queries if adding elements to
the ArrayList.
This exists only since java5, so it's not a good trick for core.
7) the org.hibernate.search.Version class:
what's the idea for the new Date() ? to append the
execution time to version number?
May I move the getLogger in the static {} code block and release the instance?
==
All above questions are about code I've already changed, just need your
opinion before commit.
Now for suggested improvements:
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.
B) there's a STATIC field in SharedReaderProvider:
"static Field subReadersField"
it is looking quite suspect, is that really meant to be static?
Also the ReaderData class contained in SharedReaderProvider
isn't looking as threadsafe; I could be wrong I only read
this class once.
I volunteer to examinate it better, also because of HSEARCH-194.
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.
D)There's a TODO somewere about making a "readonly
IndexReader"; I guess to return to users for safety
(there is a comment in the docs:
"This indexReader must not be used for modification operations
(especially delete)")
I have implemented such an IndexReader, but for visibility problems
it needs to have package "org.apache.lucene.index" ;
what do you think? Should I kindly ask to lucene developers
to add it to their package?
It's a very simple wrapper around another reader, throwing "unsupported"
on methods to be avoided.
Sanne
4:30 a.m.
Hi Sanne
On May 28, 2008, at 14:01, Sanne Grinovero wrote:
Hi Emmanuel,
(looking around the sources again) I am making several changes,
but I am feeling the urgency to ask your opinion about
"commit conventions"
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.
2)There are some "//TODO serialization" on classes that could
implement
Serializable trivially, is it ok if I just add them ?
(I'll don't do it for cases I have doubts)
I guess if I left a todo it was not so trivial at first sight and
would probably involve some transient readObject / writeObject work or
does not bring immediate usefulness. So be careful.
3)//TODO set a serial number
May I auto-generate a serialVersionUID for classes were missing,
or do you have some special policy about them I should be aware of?
I never understood that part of Java, you are free to go :)
4)final fields:
some constructor-initialized fields may need the final keyword,
especially
wen producing thread-safe transfer objects; I've added several
already,
I'm wondering now if it's ok to have this changes committed together
with 1) or if you would have preferred separated commits.
Also using "final" really helps me in better understanding the
overall code.
Separate is better, especially down the road for product maintenance.
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.
6)java.util.collections
there's a nice
List EMPTY_LIST = new ArrayList( 0 );
in QueryLoader; I would like to replace it with
java.util.Collections.EMPTY_LIST
but this has another effect: UnsupportedOperationException if someone
tries to add
elements. I think this is a good thing, but I could be wrong.
Also currently someone could reread data in other queries if adding
elements to
the ArrayList.
This exists only since java5, so it's not a good trick for core.
+1
7) the org.hibernate.search.Version class:
what's the idea for the new Date() ? to append the
execution time to version number?
May I move the getLogger in the static {} code block and release the
instance?
It's just to differentiate two snapshots and make sure they are
different from the final version. You can change the logger code if
you want
==
All above questions are about code I've already changed, just need
your
opinion before commit.
Now for suggested improvements:
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.
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.
Also the ReaderData class contained in SharedReaderProvider
isn't looking as threadsafe; I could be wrong I only read
this class once.
I volunteer to examinate it better, also because of HSEARCH-194.
I 've started providing info on HSEARCH-194 but there is a lot of work
to do
AFAIK ReaderData is protected by locks it does not need to be thread-
safe
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)
D)There's a TODO somewere about making a "readonly
IndexReader"; I guess to return to users for safety
(there is a comment in the docs:
"This indexReader must not be used for modification operations
(especially delete)")
I have implemented such an IndexReader, but for visibility problems
it needs to have package "org.apache.lucene.index" ;
what do you think? Should I kindly ask to lucene developers
to add it to their package?
It's a very simple wrapper around another reader, throwing
"unsupported"
on methods to be avoided.
Unfortunately the Lucene design lack Interfaces :)
I decided not to do the wrapper because I wanted to maximize
compatibility between different Lucene versions. But we can always
reconsider, this is not a top prioprity
Sanne
Thanks!
2:59 p.m.
Hi, I'll cut away the questions I don't need to ask more about.
2008/5/29 Emmanuel Bernard <emmanuel(a)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
4:48 p.m.
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(a)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
6:53 a.m.
> 3)//TODO set a serial number
> May I auto-generate a serialVersionUID for classes were missing,
> or do you have some special policy about them I should be aware of?
I never understood that part of Java, you are free to go :)
Please just remember that just because tools (like eclipse and others) warns about a
missing serialversionUID it
does not mean it should be there. Simon Kicthing described it well in HBX-444:
"When you declare a serialVersionUID you're also declaring that you are aware of
all the issues regarding serialising one version of a class and reimporting the serialized
data into a different version of the class, and are promising to update the
serialVersionUID whenever you make an *incompatible* change to the class.
However most people don't have a clue what is and what is not a
serialization-incompatible-change to a class. In this situation I think the default
behaviour (which is that the serialization mechanism declares incompatibility if *any*
change occurs to the class) is much safer than having an auto-generated serialVersionUID
on the class resulting in potientially corrupted objects on deserialization."
Just my 2 cents...
-max
8:31 a.m.
2008/5/30 Max Rydahl Andersen <max.andersen(a)redhat.com>:
>> 3)//TODO set a serial number
>> May I auto-generate a serialVersionUID for classes were missing,
>> or do you have some special policy about them I should be aware of?
>
> I never understood that part of Java, you are free to go :)
Please just remember that just because tools (like eclipse and others) warns about a
missing serialversionUID it
does not mean it should be there. Simon Kicthing described it well in HBX-444:
"When you declare a serialVersionUID you're also declaring that you are aware of
all the issues regarding serialising one version of a class and reimporting the serialized
data into a different version of the class, and are promising to update the
serialVersionUID whenever you make an *incompatible* change to the class.
However most people don't have a clue what is and what is not a
serialization-incompatible-change to a class. In this situation I think the default
behaviour (which is that the serialization mechanism declares incompatibility if *any*
change occurs to the class) is much safer than having an auto-generated serialVersionUID
on the class resulting in potientially corrupted objects on deserialization."
Just my 2 cents...
-max
Thanks a lot Max for clarifying,
your 2 cents are valuable.
I knew about the requirement to change the UID, but really hadn't a
clue about the default behaviour:
I thought it was "unspecified" and I really dislike that word.
I agree with you it looks like safest to generally skip it, still I
was curious about the comment
"//TODO set a serial number" : someone thinks we need one, and
probably knows better than me.
Could it be useful (or do you have any requirement) to maintain
compatibility across "some" (please define) versions?
thanks,
Sanne
12:49 p.m.
Sanne Grinovero wrote:
I knew about the requirement to change the UID, but really hadn't
a
clue about the default behaviour:
I thought it was "unspecified" and I really dislike that word.
The default behaviour is semi-specified - specifically, it's based on a
SHA1 hash of:
class name and modifiers
implemented interface names
field names, types and modifiers
(excluding 'private static' and 'private transient' fields)
presence of a static initializer
constructor signatures and modifiers
(excluding private constructors)
method names, signatures and modifiers
(excluding private methods)
The "unspecified" bit creeps in because it's not specified precisely
what synthetic members and classes used to implement language features
like class literals and inner classes should be named, so different
compiler implementations are free to make different choices, giving
different generated UIDs.
Max.
4:51 p.m.
On May 30, 2008, at 02:31, Sanne Grinovero wrote:
2008/5/30 Max Rydahl Andersen <max.andersen(a)redhat.com>:
>>> 3)//TODO set a serial number
>>> May I auto-generate a serialVersionUID for classes were missing,
>>> or do you have some special policy about them I should be aware of?
>>
>> I never understood that part of Java, you are free to go :)
>
> Please just remember that just because tools (like eclipse and
> others) warns about a missing serialversionUID it
> does not mean it should be there. Simon Kicthing described it well
> in HBX-444:
>
> "When you declare a serialVersionUID you're also declaring that you
> are aware of all the issues regarding serialising one version of a
> class and reimporting the serialized data into a different version
> of the class, and are promising to update the serialVersionUID
> whenever you make an *incompatible* change to the class.
>
> However most people don't have a clue what is and what is not a
> serialization-incompatible-change to a class. In this situation I
> think the default behaviour (which is that the serialization
> mechanism declares incompatibility if *any* change occurs to the
> class) is much safer than having an auto-generated serialVersionUID
> on the class resulting in potientially corrupted objects on
> deserialization."
>
> Just my 2 cents...
> -max
>
Thanks a lot Max for clarifying,
your 2 cents are valuable.
I knew about the requirement to change the UID, but really hadn't a
clue about the default behaviour:
I thought it was "unspecified" and I really dislike that word.
I agree with you it looks like safest to generally skip it, still I
was curious about the comment
"//TODO set a serial number" : someone thinks we need one, and
probably knows better than me.
Could it be useful (or do you have any requirement) to maintain
compatibility across "some" (please define) versions?
Simon's explanation is the reason why I usually leave it out.
That being said, the class marked as "//TODO set a serial number" is
the WorkQueue. That's a left over. It really should have been
LuceneWork which is passed along serialized in JMS (and potentially
other ways of communication).
I have removed the TODO from WorkQueue and added a clear JavaDoc in
LuceneWork
* WARNING: This class aims to be serializable and passed in an
asynchronous way across VMs
* any non backward compatible serialization change should
be done with great care
* and publically announced. Specifically, new versions of
Hibernate Search should be
* able to handle changes produced by older versions of
Hibernate Search if reasonably possible.
* That is why each subclass susceptible to be pass along
have a magic serialization number
* NOTE: we are relying on Lucene's Document to play nice
unfortunately
5809
days inactive
5811
days old
7 comments
4 participants
participants (4)
-
Emmanuel Bernard -
Max Bowsher -
Max Rydahl Andersen -
Sanne Grinovero