Make it possible to subclass SegmentReader [LUCENE-2345]

[asfimport]

I would like the ability to subclass SegmentReader for numerous reasons:

• to capture initialization/close events
• attach custom objects to an instance of a segment reader (caches, statistics, so on and so forth)
• override methods on segment reader as needed

currently this isn't really possible

I propose adding a SegmentReaderFactory that would allow creating custom subclasses of SegmentReader

default implementation would be something like:

public class SegmentReaderFactory {
  public SegmentReader get(boolean readOnly) {
    return readOnly ? new ReadOnlySegmentReader() : new SegmentReader();
  }

  public SegmentReader reopen(SegmentReader reader, boolean readOnly) {
    return newSegmentReader(readOnly);
  }
}

It would then be made possible to pass a SegmentReaderFactory to IndexWriter (for pooled readers) as well as to SegmentReader.get() (DirectoryReader.open, etc)

I could prepare a patch if others think this has merit

Obviously, this API would be "experimental/advanced/will change in future"

---

Migrated from LUCENE-2345 by Tim Smith, updated May 09 2016
Attachments: LUCENE-2345_3.0.patch, LUCENE-2345_3.0.plugins.patch

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

+1

[asfimport]

Tim Smith (migrated from JIRA)

that's the reassurance i needed :)

will start working on a patch tomorrow
will take a few days as i'll start with a 3.0 patch (which i use), then will create a 3.1 patch once i've got that all flushed out

[asfimport]

Earwin Burrfoot (migrated from JIRA)

Ahem, that directly clashes with my inflight patch?

I'm going to land create/reopen/close refactoring pretty soon, and then finish plugins.
(Which was an arduous task before, as initialization paths for SegmentReaders are really, really insane)

I'm on this patch for quite some time, I understand that, so maybe we can settle on a deadline? Don't like to see the effort vaporate, also merging is gonna be hell with flex branch alone, don't want to double it :)

[asfimport]

Tim Smith (migrated from JIRA)

i'll do my initial work on 3.0 so i can absorb the changes now and will post that patch

at which point, i can wait for you to finish whatever you need, or we can just incorporate the same ability into your patch for the other ticket
i would just like to see the ability to subclass SegmentReader's on 3.1 so i don't have to port a patch when i absorb 3.1 (just use the "finalized" apis)

[asfimport]

Tim Smith (migrated from JIRA)

Here's a patch against 3.0 that provides the SegmentReaderFactory ability
(not tested yet, but i'll be doing that shortly as i integrate this functionality)

It adds a SegmentReaderFactory.

The IndexWriter now has a getter and setter for setting this

SegmentReader has a new protected method init() which is called after the segment reader has been initialized (to allow subclasses to hook this action and do additional initialization, etc

added 2 new IndexReader.open() calls that allow specifying the SegmentReaderFactory

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

The IndexWriter now has a getter and setter for setting this

If this is not expected to change during the lifetime of IW, I think it should be added to IWC when you upgrade the patch to 3.1.

[asfimport]

Tim Smith (migrated from JIRA)

that was my plan

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I think we should only commit this only on 3.1 (new feature)?

Earwin: will this change really conflict w/ your ongoing refactoring (to have DirReader subclass MultiReader)? It seems somewhat orthogonal?

[asfimport]

Earwin Burrfoot (migrated from JIRA)

will this change really conflict w/ your ongoing refactoring (to have DirReader subclass MultiReader)? It seems somewhat orthogonal?

Ongoing contains a massive rework of how open/reopen/clone/close is done. Folding copypaste between DirReader and MultiReader is a byproduct.
The aim is to have clean initialization, get rid of of init/reinit/moreinit methods, moving the code to constructors.
This alone plays bad with factories.

The next step, plugins - conflicts ideologically. Plugins allow extension by composition, which is (in my view) much more clean than subclassing.
Landing them without refactoring all this mess beforehand is dangerous, I already hit some places where reopen/clone were done wrong in respect to existing reader fields. Even more - NRT reader lifecycle is really messy - it beginnings as a reader aimed only for merging, and then being upgraded in two separate stages is finally wrapped in DirReader to be used for actual searching. The factory/init() approach ignores this, and each user of this API will be on his own to separate lightweight readers from full-fledged ones.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Earwin, w/o knowing too much about the details of your work, I wanted to comment on "get rid of of init/reinit/moreinit methods, moving the code to constructors". I work now on Parallel Index and one of the things I do is extend IW. Currently, IW's ctor code performs the initialization, however I'm thinking to move that code to an init method. The reason is to allow easy extensions of IW, such as #3406. There I'm going to add a default ctor to IW, accompanied by an init method the extending class can call if needed. So what I'm trying to say is that init methods are not always bad, and sometimes ctors limit you. Perhaps it would make sense though in what you're trying to do ...

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Non-final/private method calls from ctors is discouraged, as it creates problems:

• If you override such a init method, you can change the behaviour of the super class' ctor
• During the time the init() method is called, the fields are not yet initialized in the subclass and also the subclass' ctor was not yet called! This leads to unexspected behaviour (like final fields changing its contents during the lifetime of the class: during the overridden init() method they are null and suddenly change to non-null after the subclass' ctor was executed, which is after the init() method)

Because of this, such init() methods should be private or at least final and never public!

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Here a good explanation: http://www.informit.com/articles/article.aspx?p=20521

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

OK, I guess for now let's hold off on this issue while we iterate at least with Earwin's first refactoring effort...?

Earwin, can you post a patch w/ your current state, even if it's "rough"? There seems to be alot of interest/opinions on how to "fix" things here :) Both IndexWriter & SegmentReader are "needing" extensibility... but this is a big change so I'm hoping we can first just get your refactoring done before adding extensibility.

Tim, do you think the plugin model ("extension by composition") would be workable for your use case? Ie, instead of a factory enabling subclasses of SegmentReader?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Thanks Uwe, I know that ctor is the preferred way, and in the process of introducing IWC I delete IW.init which all ctors called and pulled all the code to IW ctor. I will make that init() on IW final. But sometimes putting code in init() is not bad (and it's used in Lucene elsewhere too (e.g. PQ and up until recently IW).

[asfimport]

Earwin Burrfoot (migrated from JIRA)

More often than not init() methods are a sign of bad design.
I.e. in your case extending IW is crazy.
You should have an interface capturing IW methods and two implementations - one writing to the index and another delegating to its subwriters. You don't do DirectoryReader extends SegmentReader, do you? They both extend lucene-style-interface IndexReader.

Lucene's back compat policy got people used to writing and digesting freaky code, and I'm not going to fight generally against it, that's a lost cause : )
But in this exact case (readers) stuffing everything in constructors, defining all fields I can final, replacing SR.openDocStores/loadTermsIndex with reopen()-like method allows me to tackle (at least somewhat) lifecycle complexity. Javac will force me to either initialize something or explicitly leave it null.
Besides some real bugs in this code, I cleaned up cases where a field was inited twice, just in case! Poor developers got lost in init() methods : }