Lucene - Core
  1. Lucene - Core
  2. LUCENE-3776

NRTManager shouldn't expose its private SearcherManager

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff from LUCENE-3769.

      To actually obtain an IndexSearcher from NRTManager, it's a 2-step process now.

      You must .getSearcherManager(), then .acquire() from the returned SearcherManager.

      This is very trappy... because if the app incorrectly calls maybeReopen on that private SearcherManager (instead of NRTManager.maybeReopen) then it can unexpectedly cause threads to block forever, waiting for the necessary gen to become visible. This will be hard to debug... I don't like creating trappy APIs.

      Hopefully once LUCENE-3761 is in, we can fix NRTManager to no longer expose its private SM, instead subclassing ReferenceManaager.

      Or alternatively, or in addition, maybe we factor out a new interface (SearcherProvider or something...) that only has acquire and release methods, and both NRTManager and ReferenceManager/SM impl that, and we keep NRTManager's SM private.

      1. LUCENE-3776.patch
        25 kB
        Michael McCandless
      2. LUCENE-3776.patch
        20 kB
        Michael McCandless
      3. LUCENE-3776.patch
        18 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Thanks Shai!

        Show
        Michael McCandless added a comment - Thanks Shai!
        Hide
        Michael McCandless added a comment -

        Thanks Shai – I'll share a common (static) getSearcher. I think assert only makes it safer, eg in case someone changes something in the future that somehow lets a non-DR in or something...

        Show
        Michael McCandless added a comment - Thanks Shai – I'll share a common (static) getSearcher. I think assert only makes it safer, eg in case someone changes something in the future that somehow lets a non-DR in or something...
        Hide
        Shai Erera added a comment -

        Looks good Mike.

        Do we still need that assert, now that getSearcher() checks that the returned IR is the one it passed to SearcherFactory? I think it covers the case the returned IR is also a DirReader?

        assert r instanceof DirectoryReader: "searcher's IndexReader should be a DirectoryReader, but got " + r;
        

        I noticed that NRTManager shares some logic with SearcherManager (e.g. getSearcher) – is there a way to avoid the duplication? Maybe if getSearcher would be a package-private static method on SM?

        These are not critical IMO, so I'll leave the decision on whether to fix or not to you. +1 to commit.

        Show
        Shai Erera added a comment - Looks good Mike. Do we still need that assert, now that getSearcher() checks that the returned IR is the one it passed to SearcherFactory? I think it covers the case the returned IR is also a DirReader? assert r instanceof DirectoryReader: "searcher's IndexReader should be a DirectoryReader, but got " + r; I noticed that NRTManager shares some logic with SearcherManager (e.g. getSearcher) – is there a way to avoid the duplication? Maybe if getSearcher would be a package-private static method on SM? These are not critical IMO, so I'll leave the decision on whether to fix or not to you. +1 to commit.
        Hide
        Michael McCandless added a comment -

        OK, I made the evil SearcherFactory a real check; it's too dangerous if the app messes this up (eg it could be a DirReader, but a different one, and then we are inc/decRefing the wrong DR).

        I also add messages to the new asserts... I think it's ready!

        Show
        Michael McCandless added a comment - OK, I made the evil SearcherFactory a real check; it's too dangerous if the app messes this up (eg it could be a DirReader, but a different one, and then we are inc/decRefing the wrong DR). I also add messages to the new asserts... I think it's ready!
        Hide
        Shai Erera added a comment -

        Hang on – SM now takes either IW or Director

        You're right, I missed that. For some reason I had the impression it takes an IR, which is obviously wrong, since it won't be allowed to close it.

        do you mean the SearcherFactory could make some other reader

        I'm less worried about that. We give SF an IndexReader, I can only expect that it will return an IndexSearcher on top of it. Maybe we can assert that IndexSearcher.getIndexReader == newReader in refreshIfNeeded?

        I think there's no way a non-DirReader can get into NRTManager

        You're right. If you keep the assert, maybe add a nice msg to it?

        I didn't yet add a hard check for an evil SearcherFactory...

        I think that's ok to assume that SearcherFactory is not evil. Maybe the assert I suggested above would be enough?

        Show
        Shai Erera added a comment - Hang on – SM now takes either IW or Director You're right, I missed that. For some reason I had the impression it takes an IR, which is obviously wrong, since it won't be allowed to close it. do you mean the SearcherFactory could make some other reader I'm less worried about that. We give SF an IndexReader, I can only expect that it will return an IndexSearcher on top of it. Maybe we can assert that IndexSearcher.getIndexReader == newReader in refreshIfNeeded? I think there's no way a non-DirReader can get into NRTManager You're right. If you keep the assert, maybe add a nice msg to it? I didn't yet add a hard check for an evil SearcherFactory... I think that's ok to assume that SearcherFactory is not evil. Maybe the assert I suggested above would be enough?
        Hide
        Michael McCandless added a comment -

        New patch folding in Shai's suggestions (thanks!).

        I didn't yet add a hard check for an evil SearcherFactory...

        Show
        Michael McCandless added a comment - New patch folding in Shai's suggestions (thanks!). I didn't yet add a hard check for an evil SearcherFactory...
        Hide
        Michael McCandless added a comment -

        Thanks Shai.

        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.

        About openIfNeeded:
        Can you cast to DirectoryReader once?

        Will do.

        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...

        Show
        Michael McCandless added a comment - Thanks Shai. 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. About openIfNeeded: Can you cast to DirectoryReader once? Will do. 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...
        Hide
        Shai Erera added a comment -

        Patch looks good !

        SearcherManager
        with these changes, if the app passes an IndexReader that is not DirectoryReader, it will get ClassCastException (if asserts are disabled). Is that ok? Perhaps it'd be better if you check that in SM's ctor and throw IllegalArgumentException? The problem is that app cannot pass DirReader in 3x, so this will apply to trunk only. In fact, I think that for trunk it will be better if SM declared it expects a DirectoryReader up front?

        We cannot avoid the cast in refreshIfNeeded because IR is obtained from IS, but at least the app won't hit ClassCastExceptions after it created SM?

        That kinda makes SearcherManager a DirReader only impl which is unfortunate IMO. But I'm not sure if any IR can openIfChanged() anymore, so perhaps that's unavoidable.

        ReferenceManager
        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()?

        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.

        NRTManager
        About openIfNeeded:

        1. 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?
        2. 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 ).
        Show
        Shai Erera added a comment - Patch looks good ! SearcherManager with these changes, if the app passes an IndexReader that is not DirectoryReader, it will get ClassCastException (if asserts are disabled). Is that ok? Perhaps it'd be better if you check that in SM's ctor and throw IllegalArgumentException? The problem is that app cannot pass DirReader in 3x, so this will apply to trunk only. In fact, I think that for trunk it will be better if SM declared it expects a DirectoryReader up front? We cannot avoid the cast in refreshIfNeeded because IR is obtained from IS, but at least the app won't hit ClassCastExceptions after it created SM? That kinda makes SearcherManager a DirReader only impl which is unfortunate IMO. But I'm not sure if any IR can openIfChanged() anymore, so perhaps that's unavoidable. ReferenceManager 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()? 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. NRTManager About openIfNeeded: 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? 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 ).
        Hide
        Michael McCandless added a comment -

        Patch, cutting over NRTManager to subclass ReferenceManager, and also
        some minor cleanups to ReferenceManager/SearcherManager.

        I added a method, afterRefresh, to ReferenceManager, which it calls
        after a refresh; NRTManager needs this so it can
        notify any waiting threads that the new gen is now live.

        Show
        Michael McCandless added a comment - Patch, cutting over NRTManager to subclass ReferenceManager, and also some minor cleanups to ReferenceManager/SearcherManager. I added a method, afterRefresh, to ReferenceManager, which it calls after a refresh; NRTManager needs this so it can notify any waiting threads that the new gen is now live.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development