SegmentReader changes?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

SegmentReader changes?

Robert Engels
I think one of two things need to happen to the SegmentReader class.

Either make the 'segment' variable protected, or make the the initialize()
method protected.

Without this, subclassing SegmentReader is impossible, since there is no way
for the derived class to know what segment it is working with.

In implementing the 'reopen()' method SegmentReader needs to be subclassed
in order to support 'refreshing' the deleted documents.
Reply | Threaded
Open this post in threaded view
|

Re: SegmentReader changes?

Doug Cutting
Robert Engels wrote:
> In implementing the 'reopen()' method SegmentReader needs to be subclassed
> in order to support 'refreshing' the deleted documents.

Why subclass?  Why not simply change SegmentReader?  It's not a public
class at present, and making it a public class would be a bigger change
than should be required to implement reopen.

But perhaps I just don't yet understand how you intend to implement
re-open.  I think I'd implement it as something that inquired whether
the deletions have changed, and if they have, clone the SegmentReader,
re-opening all files, but only re-reading the deletions.

Doug

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: SegmentReader changes?

Robert Engels
Correct - changing SegmentReader would be best, but in the past, getting
proposed patches included has been slower than expected. So, by making the
SegmentReader more easily subclassed (which should hopefully get approved
quicker), I can still have a "build" of Lucene that does not require
patching any files. (just including classes in the appropriate package to
access package level vars/methods).

I can do everything needed (without subclassing) if there was a
package/public accessor to the segment "name".

-----Original Message-----
From: Doug Cutting [mailto:[hidden email]]
Sent: Monday, May 01, 2006 5:44 PM
To: [hidden email]
Subject: Re: SegmentReader changes?


Robert Engels wrote:
> In implementing the 'reopen()' method SegmentReader needs to be subclassed
> in order to support 'refreshing' the deleted documents.

Why subclass?  Why not simply change SegmentReader?  It's not a public
class at present, and making it a public class would be a bigger change
than should be required to implement reopen.

But perhaps I just don't yet understand how you intend to implement
re-open.  I think I'd implement it as something that inquired whether
the deletions have changed, and if they have, clone the SegmentReader,
re-opening all files, but only re-reading the deletions.

Doug

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: SegmentReader changes?

Doug Cutting
Robert Engels wrote:
> Correct - changing SegmentReader would be best, but in the past, getting
> proposed patches included has been slower than expected.

I'm sorry if the process has been frustrating to you in the past.  I
hope your experiences are better in the future.

> So, by making the
> SegmentReader more easily subclassed (which should hopefully get approved
> quicker), I can still have a "build" of Lucene that does not require
> patching any files. (just including classes in the appropriate package to
> access package level vars/methods).

Aren't we discussing a change to IndexReader, adding a new method?  This
is not a contrib module, but a change to the core.  So proposing it as a
patch file that changes existing classes is the normal course.  I don't
think we ought to be in the pracice of making changes in order to
support easier access to non-public APIs.

Doug

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: SegmentReader changes?

Robert Engels
I can submit a patch to add the IndexReader.reopen() method.

BUT, I think the requested change to SegmentReader is still valid, for the
reasons cited in the previous email.

There is already support for replacing the SegmentReader impl at runtime
with System properties, but without the SegmentReader changes I think it is
next to impossible to have any worthwhile subclass - except for "maybe"
method logging, so either the runtime replacement code should be removed, or
the changes made. Currently there just isn't a way for the subclass to know
ANYTHING, since all of the initialization methods called by the static
factory method are private.

-----Original Message-----
From: Doug Cutting [mailto:[hidden email]]
Sent: Monday, May 01, 2006 6:03 PM
To: [hidden email]
Subject: Re: SegmentReader changes?


Robert Engels wrote:
> Correct - changing SegmentReader would be best, but in the past, getting
> proposed patches included has been slower than expected.

I'm sorry if the process has been frustrating to you in the past.  I
hope your experiences are better in the future.

> So, by making the
> SegmentReader more easily subclassed (which should hopefully get approved
> quicker), I can still have a "build" of Lucene that does not require
> patching any files. (just including classes in the appropriate package to
> access package level vars/methods).

Aren't we discussing a change to IndexReader, adding a new method?  This
is not a contrib module, but a change to the core.  So proposing it as a
patch file that changes existing classes is the normal course.  I don't
think we ought to be in the pracice of making changes in order to
support easier access to non-public APIs.

Doug

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: SegmentReader changes?

Doug Cutting
If the non-public core requires a subclassible SegmentReader then
SegmentReader should certainly be made subclassible.  But we shouldn't
make changes to improve the extensibility of the non-public API.  That's
a slipperly slope.  The fact that you can access package-protected
members by writing code in the same package is a loophole, not a
supported extension mechanism.  We need to retain the freedom to change
non-public APIs without warning.

I'd love to see a good patch that adds an IndexReader.reopen() method
and I hope you are not discouraged from writing one.

Doug

Robert Engels wrote:

> I can submit a patch to add the IndexReader.reopen() method.
>
> BUT, I think the requested change to SegmentReader is still valid, for the
> reasons cited in the previous email.
>
> There is already support for replacing the SegmentReader impl at runtime
> with System properties, but without the SegmentReader changes I think it is
> next to impossible to have any worthwhile subclass - except for "maybe"
> method logging, so either the runtime replacement code should be removed, or
> the changes made. Currently there just isn't a way for the subclass to know
> ANYTHING, since all of the initialization methods called by the static
> factory method are private.
>
> -----Original Message-----
> From: Doug Cutting [mailto:[hidden email]]
> Sent: Monday, May 01, 2006 6:03 PM
> To: [hidden email]
> Subject: Re: SegmentReader changes?
>
>
> Robert Engels wrote:
>
>>Correct - changing SegmentReader would be best, but in the past, getting
>>proposed patches included has been slower than expected.
>
>
> I'm sorry if the process has been frustrating to you in the past.  I
> hope your experiences are better in the future.
>
>
>>So, by making the
>>SegmentReader more easily subclassed (which should hopefully get approved
>>quicker), I can still have a "build" of Lucene that does not require
>>patching any files. (just including classes in the appropriate package to
>>access package level vars/methods).
>
>
> Aren't we discussing a change to IndexReader, adding a new method?  This
> is not a contrib module, but a change to the core.  So proposing it as a
> patch file that changes existing classes is the normal course.  I don't
> think we ought to be in the pracice of making changes in order to
> support easier access to non-public APIs.
>
> Doug
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: SegmentReader changes?

Robert Engels
No, not at all. I will put something together.

BUT, back to the subclassing comments... Why have the runtime replaceable
support then in the SegmentReader factory - there is nothing useful a
subclass can do at this time, and without API changes, it will never be able
to.



-----Original Message-----
From: Doug Cutting [mailto:[hidden email]]
Sent: Monday, May 01, 2006 6:22 PM
To: [hidden email]
Subject: Re: SegmentReader changes?


If the non-public core requires a subclassible SegmentReader then
SegmentReader should certainly be made subclassible.  But we shouldn't
make changes to improve the extensibility of the non-public API.  That's
a slipperly slope.  The fact that you can access package-protected
members by writing code in the same package is a loophole, not a
supported extension mechanism.  We need to retain the freedom to change
non-public APIs without warning.

I'd love to see a good patch that adds an IndexReader.reopen() method
and I hope you are not discouraged from writing one.

Doug

Robert Engels wrote:
> I can submit a patch to add the IndexReader.reopen() method.
>
> BUT, I think the requested change to SegmentReader is still valid, for the
> reasons cited in the previous email.
>
> There is already support for replacing the SegmentReader impl at runtime
> with System properties, but without the SegmentReader changes I think it
is
> next to impossible to have any worthwhile subclass - except for "maybe"
> method logging, so either the runtime replacement code should be removed,
or
> the changes made. Currently there just isn't a way for the subclass to
know

> ANYTHING, since all of the initialization methods called by the static
> factory method are private.
>
> -----Original Message-----
> From: Doug Cutting [mailto:[hidden email]]
> Sent: Monday, May 01, 2006 6:03 PM
> To: [hidden email]
> Subject: Re: SegmentReader changes?
>
>
> Robert Engels wrote:
>
>>Correct - changing SegmentReader would be best, but in the past, getting
>>proposed patches included has been slower than expected.
>
>
> I'm sorry if the process has been frustrating to you in the past.  I
> hope your experiences are better in the future.
>
>
>>So, by making the
>>SegmentReader more easily subclassed (which should hopefully get approved
>>quicker), I can still have a "build" of Lucene that does not require
>>patching any files. (just including classes in the appropriate package to
>>access package level vars/methods).
>
>
> Aren't we discussing a change to IndexReader, adding a new method?  This
> is not a contrib module, but a change to the core.  So proposing it as a
> patch file that changes existing classes is the normal course.  I don't
> think we ought to be in the pracice of making changes in order to
> support easier access to non-public APIs.
>
> Doug
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: SegmentReader changes?

Doug Cutting
Robert Engels wrote:
> BUT, back to the subclassing comments... Why have the runtime replaceable
> support then in the SegmentReader factory - there is nothing useful a
> subclass can do at this time, and without API changes, it will never be able
> to.

That's all true.  I don't dispute it.  There's an inconsistency in an
undocumented non-public feature.  But that alone does not motivate
changing anything, since it's not public and it's not a bug, merely an
inconvenience for someone trying to do something they shouldn't.
Something else needs to motivate changing it, like a patch that
implements IndexReader.reopen() that requires a subclassable
SegmentReader.  Or some committer needs to be motivated to make such an
aesthetic cleanup.  I, today, am not that committer.

Doug

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]