with these changes, if the app passes an IndexReader that is not DirectoryReader, it will get ClassCastException (if asserts are disabled).
Hang on – SM now takes either IW or Directory, from which we always
pull a DirectoryReader, right (we call DR.open ourselves)? Won't we
always have a DR in SM...?
Hmm, or do you mean the SearcherFactory could make some other
reader...? Hmm maybe we should have a hard check for that
(SearcherFactory shouldn't do that...?)
About close() – do you think it'll be better to keep close() final, and introduce a new protected closeResource()/closeInternal() that NRTManager can override? That way, RefManagers won't accidentally override close() and forget to call super.close()?
Good idea... I'll add afterClose (matches afterRefresh);
About afterRefresh() – I'll admit that first I didn't understand why you need it. Previously, it was used to warm an IndexSearcher, but now we say it's the responsibility of SearcherFactory. I can see why it's useful for NRTManager, and it might even help me in
LUCENE-3793 ! Do you think that we should declare that it can throw IOE? I know that if I'll use it in LUCENE-3793, I'll need that and I'd hate to throw RuntimeException. NRTManager can still override and not declare that. I'm just thinking that since almost all methods declare throwing IOE, it won't be odd if we declare it too on afterRefresh(), and it's not unlikely that afterRefresh() will do something that throws exceptions.
Good, I'll add throws IOE.
Can you cast to DirectoryReader once?
I don't know if the assert is better than a ClassCastException ... with how the code is written, ClassCastException is better than assert because at least it will tell the user what went wrong?
I think there's no way a non-DirReader can get into NRTManager (like
SM), except for SearcherFactory.
How critical it is to declare newSearcher final? If you didn't, you could init it to null, and only change if newReader != null. Saving 4 lines of code (improves readability IMO – something that I know you care about ).
Not critical! Good idea...