Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-5889

AnalyzingInfixSuggester should expose commit()

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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
          varunthacker 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
          varunthacker 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
          mikemccand 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
          mikemccand 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
          varunthacker Varun Thacker added a comment -

          Thanks for the review!

          New patch which addresses all the inputs you provided.

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

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

          Show
          mikemccand Michael McCandless added a comment - Thanks Varun, looks good, I'll commit. I'll just fix whitespace: "if(" should be "if ("
          Hide
          jira-bot 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
          jira-bot 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
          jira-bot 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
          jira-bot 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
          mikemccand Michael McCandless added a comment -

          Thanks Varun!

          Show
          mikemccand Michael McCandless added a comment - Thanks Varun!
          Hide
          hossman 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
          hossman 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
          markrmiller@gmail.com Mark Miller added a comment -

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

          Show
          markrmiller@gmail.com Mark Miller added a comment - Is there a thread safety issue with the writer in AnalyzingInfixSuggester here?
          Hide
          rcmuir 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
          rcmuir 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 Areek Zillur added a comment -

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

          Show
          areek Areek Zillur added a comment - I am currently investigating the problem, will update when I find anything interesting.
          Hide
          hossman 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
          hossman 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
          markrmiller@gmail.com 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
          markrmiller@gmail.com 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 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 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
          hossman 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
          hossman 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
          elyograg 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
          elyograg 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
          mikemccand Michael McCandless added a comment -

          Grr, sorry for all the noise here

          I'll fix the javadocs smoke test failure.

          Show
          mikemccand Michael McCandless added a comment - Grr, sorry for all the noise here I'll fix the javadocs smoke test failure.
          Hide
          jira-bot 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
          jira-bot 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
          jira-bot 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
          jira-bot 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
          sokolov 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
          sokolov 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
          mikemccand 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
          mikemccand 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
          anshumg Anshum Gupta added a comment -

          Bulk close after 5.0 release.

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development