Lucene - Core
  1. Lucene - Core
  2. LUCENE-5889

AnalyzingInfixSuggester should expose commit()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: modules/spellchecker
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      There is no way short of close() for a user of AnalyzingInfixSuggester to cause it to commit() its underlying index: only refresh() is provided. But callers might want to ensure the index is flushed to disk without closing.

      1. LUCENE-5889.patch
        24 kB
        Varun Thacker
      2. LUCENE-5889.patch
        5 kB
        Varun Thacker

        Issue Links

          Activity

          Hide
          Varun Thacker added a comment -

          Patch which exposes commit.

          I also added param to the constructor 'commitOnBuild' . I felt it was a good option to have and especially useful in Solr where we just expose the build() method for all suggesters.

          Michael McCandless - Also regarding your comments on the user list regarding the NPE's in add(), update() I think it would be more convenient for a user to keep calling add() in a loop and then call commit() to build his suggester. It saves the user the hassle of creating a Iterator. If you think otherwise I could change it to IllegalStateException.

          Show
          Varun Thacker added a comment - Patch which exposes commit. I also added param to the constructor 'commitOnBuild' . I felt it was a good option to have and especially useful in Solr where we just expose the build() method for all suggesters. Michael McCandless - Also regarding your comments on the user list regarding the NPE's in add(), update() I think it would be more convenient for a user to keep calling add() in a loop and then call commit() to build his suggester. It saves the user the hassle of creating a Iterator. If you think otherwise I could change it to IllegalStateException.
          Hide
          Michael McCandless added a comment -

          Thanks Varun!

          I think we shouldn't add another ctor? (The API is experimental.).
          Just add the new commitOnBuild param ...

          I think it's fine to init the writer in add/update, but can you pull
          that out to its own method, e.g. getWriter? And I guess it should be
          sync'd, in case multiple threads try to add/update at once?

          And maybe add some tests showing that you can just use .add and then
          refresh and see the suggestions?

          Show
          Michael McCandless added a comment - Thanks Varun! I think we shouldn't add another ctor? (The API is experimental.). Just add the new commitOnBuild param ... I think it's fine to init the writer in add/update, but can you pull that out to its own method, e.g. getWriter? And I guess it should be sync'd, in case multiple threads try to add/update at once? And maybe add some tests showing that you can just use .add and then refresh and see the suggestions?
          Hide
          Varun Thacker added a comment -

          Thanks for the review!

          New patch which addresses all the inputs you provided.

          Show
          Varun Thacker added a comment - Thanks for the review! New patch which addresses all the inputs you provided.
          Hide
          Michael McCandless added a comment -

          Thanks Varun, looks good, I'll commit. I'll just fix whitespace: "if(" should be "if ("

          Show
          Michael McCandless added a comment - Thanks Varun, looks good, I'll commit. I'll just fix whitespace: "if(" should be "if ("
          Hide
          ASF subversion and git services added a comment -

          Commit 1619635 from Michael McCandless in branch 'dev/trunk'
          [ https://svn.apache.org/r1619635 ]

          LUCENE-5889: add commit method to AnalyzingInfixSuggester

          Show
          ASF subversion and git services added a comment - Commit 1619635 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1619635 ] LUCENE-5889 : add commit method to AnalyzingInfixSuggester
          Hide
          ASF subversion and git services added a comment -

          Commit 1619636 from Michael McCandless in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1619636 ]

          LUCENE-5889: add commit method to AnalyzingInfixSuggester

          Show
          ASF subversion and git services added a comment - Commit 1619636 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1619636 ] LUCENE-5889 : add commit method to AnalyzingInfixSuggester
          Hide
          Michael McCandless added a comment -

          Thanks Varun!

          Show
          Michael McCandless added a comment - Thanks Varun!
          Hide
          Hoss Man added a comment - - edited

          The backport of this issue seems to be breaking tests on the 4x branch

          comments from email (Subject: Re: [JENKINS] Lucene-Solr-Tests-4.x-Java7 - Build # 2066 - Failure)...

          Something on 4.x changed in the last ~24 hours or so has started causing 
          all solr "Suggest" based tests to fail reliably, regardless of seed...
          
          cd solr/core && ant test -Dtests.class=\*Suggest\*
          ...
             [junit4] Tests with failures:
             [junit4]   - org.apache.solr.spelling.suggest.TestAnalyzedSuggestions (suite)
             [junit4]   - org.apache.solr.spelling.suggest.TestAnalyzeInfixSuggestions (suite)
             [junit4]   - org.apache.solr.spelling.suggest.TestFuzzyAnalyzedSuggestions (suite)
             [junit4]   - org.apache.solr.spelling.suggest.TestPhraseSuggestions (suite)
          
          
          The common problem seems to be related to SolrCore init? ...
          
          org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: null
          ...
          Caused by: java.lang.RuntimeException
                  at org.apache.solr.spelling.suggest.fst.BlendedInfixLookupFactory.create(BlendedInfixLookupFactory.java:102)
                  at org.apache.solr.spelling.suggest.SolrSuggester.init(SolrSuggester.java:104)
          
          
          ...anybody have any idea what happened here?
          
          : Something on 4.x changed in the last ~24 hours or so has started causing 
          : all solr "Suggest" based tests to fail reliably, regardless of seed...
          
          Hmmm... this fauses several failures for me everytime...
          
          ant test  -Dtests.class=\*Suggest\*
          
          But try any indiividual "reproduce with" line from those failures and they 
          pass.
          
          
          A quick poke at the code suggests that there is a problem of leaking 
          write locks between tests?
          
          
          There's some stupid exception handling in the solr factories thats masking 
          the root problem by just throwing "new RuntimeException()", but it seems 
          to be that in all the failures the calls like this...
          
                return new BlendedInfixSuggester(core.getSolrConfig().luceneMatchVersion, 
                                                 FSDirectory.open(new File(indexPath)),
                                                 indexAnalyzer, queryAnalyzer, minPrefixChars,
                                                 blenderType, numFactor, true);
          
          ...are throwing IOExceptions due ot write.lock failures
          
             [junit4]    > Caused by: 
          org.apache.lucene.store.LockObtainFailedException: Lock obtain timed out: 
          NativeFSLock@/home/hossman/lucene/4x_dev/solr/build/solr-core/test/J1/blendedInfixDir1/write.lock
             [junit4]    >        at 
          org.apache.lucene.store.Lock.obtain(Lock.java:89)
             [junit4]    >        at 
          org.apache.lucene.index.IndexWriter.<init>(IndexWriter.java:756)
             [junit4]    >        at 
          org.apache.lucene.search.suggest.analyzing.AnalyzingInfixSuggester.<init>(AnalyzingInfixSuggester.java:185)
             [junit4]    >        at 
          org.apache.lucene.search.suggest.analyzing.BlendedInfixSuggester.<init>(BlendedInfixSuggester.java:115)
             [junit4]    >        at 
          org.apache.solr.spelling.suggest.fst.BlendedInfixLookupFactory.create(BlendedInfixLookupFactory.java:97)
             [junit4]    >        ... 12 more
          
          
          ...which smells like SOLR-6246, except that these tests aren't doing core 
          reloads -- so who's got the write lock?
          
          Looking at recent commits, LUCENE-5889 seems like the most likeley culprit 
          for potentially introducing a related bug -- when i reverted it on 4x 
          ("svn merge -c -1619636 ." as of r1619866) these tests started passing 
          again.
          
          Show
          Hoss Man added a comment - - edited The backport of this issue seems to be breaking tests on the 4x branch comments from email ( Subject: Re: [JENKINS] Lucene-Solr-Tests-4.x-Java7 - Build # 2066 - Failure )... Something on 4.x changed in the last ~24 hours or so has started causing all solr "Suggest" based tests to fail reliably, regardless of seed... cd solr/core && ant test -Dtests.class=\*Suggest\* ... [junit4] Tests with failures: [junit4] - org.apache.solr.spelling.suggest.TestAnalyzedSuggestions (suite) [junit4] - org.apache.solr.spelling.suggest.TestAnalyzeInfixSuggestions (suite) [junit4] - org.apache.solr.spelling.suggest.TestFuzzyAnalyzedSuggestions (suite) [junit4] - org.apache.solr.spelling.suggest.TestPhraseSuggestions (suite) The common problem seems to be related to SolrCore init? ... org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: null ... Caused by: java.lang.RuntimeException at org.apache.solr.spelling.suggest.fst.BlendedInfixLookupFactory.create(BlendedInfixLookupFactory.java:102) at org.apache.solr.spelling.suggest.SolrSuggester.init(SolrSuggester.java:104) ...anybody have any idea what happened here? : Something on 4.x changed in the last ~24 hours or so has started causing : all solr "Suggest" based tests to fail reliably, regardless of seed... Hmmm... this fauses several failures for me everytime... ant test -Dtests.class=\*Suggest\* But try any indiividual "reproduce with" line from those failures and they pass. A quick poke at the code suggests that there is a problem of leaking write locks between tests? There's some stupid exception handling in the solr factories thats masking the root problem by just throwing "new RuntimeException()", but it seems to be that in all the failures the calls like this... return new BlendedInfixSuggester(core.getSolrConfig().luceneMatchVersion, FSDirectory.open(new File(indexPath)), indexAnalyzer, queryAnalyzer, minPrefixChars, blenderType, numFactor, true); ...are throwing IOExceptions due ot write.lock failures [junit4] > Caused by: org.apache.lucene.store.LockObtainFailedException: Lock obtain timed out: NativeFSLock@/home/hossman/lucene/4x_dev/solr/build/solr-core/test/J1/blendedInfixDir1/write.lock [junit4] > at org.apache.lucene.store.Lock.obtain(Lock.java:89) [junit4] > at org.apache.lucene.index.IndexWriter.<init>(IndexWriter.java:756) [junit4] > at org.apache.lucene.search.suggest.analyzing.AnalyzingInfixSuggester.<init>(AnalyzingInfixSuggester.java:185) [junit4] > at org.apache.lucene.search.suggest.analyzing.BlendedInfixSuggester.<init>(BlendedInfixSuggester.java:115) [junit4] > at org.apache.solr.spelling.suggest.fst.BlendedInfixLookupFactory.create(BlendedInfixLookupFactory.java:97) [junit4] > ... 12 more ...which smells like SOLR-6246, except that these tests aren't doing core reloads -- so who's got the write lock? Looking at recent commits, LUCENE-5889 seems like the most likeley culprit for potentially introducing a related bug -- when i reverted it on 4x ("svn merge -c -1619636 ." as of r1619866) these tests started passing again.
          Hide
          Mark Miller added a comment -

          Is there a thread safety issue with the writer in AnalyzingInfixSuggester here?

          Show
          Mark Miller added a comment - Is there a thread safety issue with the writer in AnalyzingInfixSuggester here?
          Hide
          Robert Muir added a comment -

          FYI Mike just left on vacation for a while. I'm not familiar with the issue yet so unable to help right now myself (currently busy).

          Show
          Robert Muir added a comment - FYI Mike just left on vacation for a while. I'm not familiar with the issue yet so unable to help right now myself (currently busy).
          Hide
          Areek Zillur added a comment -

          I am currently investigating the problem, will update when I find anything interesting.

          Show
          Areek Zillur added a comment - I am currently investigating the problem, will update when I find anything interesting.
          Hide
          Hoss Man added a comment -

          Is there a thread safety issue with the writer in AnalyzingInfixSuggester here?

          there's definitely a lock not getting freed somewhere - but i don't know about thread safety.

          I am currently investigating the problem, will update when I find anything interesting.

          Thanks areek.

          NOTE: it's totally possible that this change itself is fine and correct, and just happened to tickle some existing bug in the Solr factories that wrap the suggesters

          Show
          Hoss Man added a comment - Is there a thread safety issue with the writer in AnalyzingInfixSuggester here? there's definitely a lock not getting freed somewhere - but i don't know about thread safety. I am currently investigating the problem, will update when I find anything interesting. Thanks areek. NOTE: it's totally possible that this change itself is fine and correct, and just happened to tickle some existing bug in the Solr factories that wrap the suggesters
          Hide
          Mark Miller added a comment -

          but i don't know about thread safety.

          It's hard to know at a glance, but the class is written in a way that says it expects to be used by multiple threads and the indexwriter is not accessed in a way that expects to be used by multiple threads. No fire, but a bunch of smoke.

          Show
          Mark Miller added a comment - but i don't know about thread safety. It's hard to know at a glance, but the class is written in a way that says it expects to be used by multiple threads and the indexwriter is not accessed in a way that expects to be used by multiple threads. No fire, but a bunch of smoke.
          Hide
          Areek Zillur added a comment - - edited

          Interesting: Both AnalyzingInfixSuggester & BlendedInfixSuggester implements Closeable and on close closes the writer and searchmanager it uses. But looking into the Solr factories and suggest component, there seems to be no way to close a suggester (close() is never callled). What might happen is, as all the failing tests re-uses the same 'solrconfig-phrasesuggest.xml', the *InfixSuggester never closes the writer or the searchmanager and on subsequent instantiations throws the LockObtainFailedException (as all tests are re-using the same directory path for the underlying index)?

          Show
          Areek Zillur added a comment - - edited Interesting: Both AnalyzingInfixSuggester & BlendedInfixSuggester implements Closeable and on close closes the writer and searchmanager it uses. But looking into the Solr factories and suggest component, there seems to be no way to close a suggester (close() is never callled). What might happen is, as all the failing tests re-uses the same 'solrconfig-phrasesuggest.xml', the *InfixSuggester never closes the writer or the searchmanager and on subsequent instantiations throws the LockObtainFailedException (as all tests are re-using the same directory path for the underlying index)?
          Hide
          Hoss Man added a comment - - edited

          But looking into the Solr factories and suggest component, there seems to be no way to close a suggester (close() is never callled).

          Huh?

          Awe, damn it ... on trunk Suggester registers a CloseHook for the Lookup, but apparently that never got backported to 4x.

          So it looks like this is in fact a bug in Solr's Suggester class and we've just ben (un)lucky that are tests never triggered it, but the new logic added here (aparently) shifts when the write locks are held, so w/o the CloseHook the tests are failing.

          I'll open a new Jira: SOLR-5322

          (Thanks Areek!)

          Show
          Hoss Man added a comment - - edited But looking into the Solr factories and suggest component, there seems to be no way to close a suggester (close() is never callled). Huh? Awe, damn it ... on trunk Suggester registers a CloseHook for the Lookup, but apparently that never got backported to 4x. So it looks like this is in fact a bug in Solr's Suggester class and we've just ben (un)lucky that are tests never triggered it, but the new logic added here (aparently) shifts when the write locks are held, so w/o the CloseHook the tests are failing. I'll open a new Jira: SOLR-5322 (Thanks Areek!)
          Hide
          Shawn Heisey added a comment -

          There's a precommit failure for missing javadoc on AnalyzingInfixSuggester#commit(), added by the svn commit for this issue. If I add a dummy javadoc to the method, precommit passes. I would fix, but I don't really know what the javadoc should say, and I'd rather not add something that's incorrect or incomplete.

          Show
          Shawn Heisey added a comment - There's a precommit failure for missing javadoc on AnalyzingInfixSuggester#commit(), added by the svn commit for this issue. If I add a dummy javadoc to the method, precommit passes. I would fix, but I don't really know what the javadoc should say, and I'd rather not add something that's incorrect or incomplete.
          Hide
          Michael McCandless added a comment -

          Grr, sorry for all the noise here

          I'll fix the javadocs smoke test failure.

          Show
          Michael McCandless added a comment - Grr, sorry for all the noise here I'll fix the javadocs smoke test failure.
          Hide
          ASF subversion and git services added a comment -

          Commit 1620180 from Michael McCandless in branch 'dev/trunk'
          [ https://svn.apache.org/r1620180 ]

          LUCENE-5889: add missing javadocs

          Show
          ASF subversion and git services added a comment - Commit 1620180 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1620180 ] LUCENE-5889 : add missing javadocs
          Hide
          ASF subversion and git services added a comment -

          Commit 1620182 from Michael McCandless in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1620182 ]

          LUCENE-5889: add missing javadocs

          Show
          ASF subversion and git services added a comment - Commit 1620182 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1620182 ] LUCENE-5889 : add missing javadocs
          Hide
          Mike Sokolov added a comment -

          It seems like my issues got addressed here - thanks! It looks like possibly this is ready for backporting to 4.x now? I would love to see this in a release if anyone has a moment to merge it in.

          Show
          Mike Sokolov added a comment - It seems like my issues got addressed here - thanks! It looks like possibly this is ready for backporting to 4.x now? I would love to see this in a release if anyone has a moment to merge it in.
          Hide
          Michael McCandless added a comment -

          I think we shouldn't backport commit to 4.10.x? That branch is only for bug fixes... hopefully we soon release 5.0, which has the new commit method.

          Show
          Michael McCandless added a comment - I think we shouldn't backport commit to 4.10.x? That branch is only for bug fixes... hopefully we soon release 5.0, which has the new commit method.
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Unassigned
              Reporter:
              Mike Sokolov
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development