Re: DO NOT REPLY [Bug 30736] - [PATCH] to remove synchronized code from TermVectorsReader

Thu, 19 Aug 2004 14:44:43 -0700 

Otis,

sorry to say, the implementation i with provided with that patch would work, but looking in detail on it, it's simply bullshit.

It's not just the IOException which is caught and not passed to the caller. The current implementation would open InputStreams for each thread which never get closed (thanks Christoph for the tip). Every thread calling the ThreadLocal.get() method is creating a new TermVectorsReader within the anonymous inner class. The opened InputStreams in TermVectorsReader will never get closed again (correct me please if I'm wrong). So the only way i see, is to make the TermVectorReader class cloneable and put a clone of the original into the ThreadLocal.

The implementation i have in mind, would look very similar to the one Doug introduced in TermInfosReader, handling the SegmentTermEnum objects.

So please, simply forget that bad shot. I'll gonna try to correct it and add the new files to the current patch in Bugzilla.

thx
Bernhard

Otis Gospodnetic wrote:

Ah, I see English.java.  I didn't check test/ directories.

IOException - yes, let the caller deal with it.

Please just attach new diffs to existing Bugzilla entry.  I'll ignore
the old ones.

Thanks,
Otis


--- Bernhard Messer <[EMAIL PROTECTED]> wrote:



Otis,

the English class is in cvs, that's where i found it. It is also used
by other test classes like TestTermVectors e.g.

The IOException was something where i wasn't sure how to process. I think you're right, the best idea would be to pop it up to the
caller. Looking at the original code, the IOException wasn't caught in TermVectors constructor.

Sorry about the tabs, this are my settings in exlipse, inserting tabs

instead of blanks.

Shall i create a new patch send it to the list, or do i have to
create a new bugzilla issue for that. Is it possible to update attachments in bugzilla ? Don't think so.

regards
Bernhard

[EMAIL PROTECTED] wrote:



DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=30736>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=30736

[PATCH] to remove synchronized code from TermVectorsReader





------- Additional Comments From [EMAIL PROTECTED] 2004-08-19 11:47


-------


Bernhard,

Thanks for the patch. The unit test requires class


o.a.lucene.util.English.

This is not in CVS. Is this something that should be in the CVS?

What is it?


I am also wondering about this piece of code:

- termVectorsReader = new TermVectorsReader(cfsDir, segment,


fieldInfos);


+ final Directory dir = cfsDir;
+ termVectorsLocal = new ThreadLocal() {
+ protected synchronized Object initialValue() {
+ try {
+ return new TermVectorsReader(dir,


segment,


fieldInfos);
+                       } catch (IOException ioe) {
+                               ioe.printStackTrace();
+                               return null;
+                       }
+               }
+       };

Is is a good thing to 'eat' that IOException and quietly return


null? The


method where this code is, is already throwing IOException, so why


not let the


IOException pop up?

Finally, it looks like diffs contain tabs. Could you please change


tabs to 2


spaces?

Thanks,
Otis



---------------------------------------------------------------------


To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]





---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]






---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to