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

Simplify or remove use of Version in IndexWriterConfig

    Details

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

      Description

      IndexWriter currently uses Version from IndexWriterConfig to determine the semantics of close(). This is a trapdoor for users, as they often default to just sending Version.LUCENE_CURRENT since they don't understand what it will be used for. Instead, we should make the semantics of close a direction option in IWC.

      1. LUCENE-5871.iwclose.4x.patch
        20 kB
        Michael McCandless
      2. LUCENE-5871.iwclose.trunk.patch
        8 kB
        Michael McCandless
      3. LUCENE-5871.patch
        820 kB
        Ryan Ernst
      4. LUCENE-5871.patch
        819 kB
        Ryan Ernst
      5. LUCENE-5871.patch
        709 kB
        Michael McCandless
      6. LUCENE-5871.patch
        696 kB
        Ryan Ernst
      7. LUCENE-5871.patch
        181 kB
        Ryan Ernst

        Issue Links

          Activity

          Hide
          shaie Shai Erera added a comment -

          Version is currently used to decide whether to throw an exception in case IW has uncommitted or in-flight merges when you call close(), or silently discard them. I think that users who go to 5.0 will need to read the CHANGES carefully anyway, and adapt to the new way (close() only closes, not commit). I think we added it as convenience for users who upgrade and don't read CHANGES. But as you say, those users probably also pass Version.CURRENT, which means they silently lose those changes, so I now wonder if we should have this "convenience" in the code at all. And if we do decide to keep it, I suggest deprecating it in trunk, so it can be removed in e.g. 6.0.

          Show
          shaie Shai Erera added a comment - Version is currently used to decide whether to throw an exception in case IW has uncommitted or in-flight merges when you call close(), or silently discard them. I think that users who go to 5.0 will need to read the CHANGES carefully anyway, and adapt to the new way (close() only closes, not commit). I think we added it as convenience for users who upgrade and don't read CHANGES. But as you say, those users probably also pass Version.CURRENT, which means they silently lose those changes, so I now wonder if we should have this "convenience" in the code at all. And if we do decide to keep it, I suggest deprecating it in trunk, so it can be removed in e.g. 6.0.
          Hide
          rjernst Ryan Ernst added a comment -

          I don't think we should deprecate something that is only in trunk. We should simply change trunk, as 5.0 is not released. I am suggesting replacing the logic depending on version with an explicit option in IWC. It will default to current behavior (as if we saw LUCENE_5_0 currently), but can be set to use the semantics of 4x (commit on close).

          Show
          rjernst Ryan Ernst added a comment - I don't think we should deprecate something that is only in trunk. We should simply change trunk, as 5.0 is not released. I am suggesting replacing the logic depending on version with an explicit option in IWC. It will default to current behavior (as if we saw LUCENE_5_0 currently), but can be set to use the semantics of 4x (commit on close).
          Hide
          rjernst Ryan Ernst added a comment -

          Here is a patch with the plan as I described above. All tests pass.

          Show
          rjernst Ryan Ernst added a comment - Here is a patch with the plan as I described above. All tests pass.
          Hide
          shaie Shai Erera added a comment -

          I don't think this patch is correct. IndexWriter uses Version not in order to commitOnClose, but to throw an exception if you have uncommitted changes. You see, IndexWriter.close() no longer commits in trunk, it always discards uncommitted changes. The only difference is whether they are discarded silently or loudly.

          That's why I said that this code should be either removed or deprecated in 5.0 and removed in 6.0. The only reason this code exists is that we want people who migrate from 4.x to 5.0 will not silently break their app if they didn't call commit() prior to close() (or called shutdown()). But I'm not sure these users will know to pass Version < 5.0 in order to set that behavior, unless they already do.

          Anyway, if we want to make this patch valid, we should name the setter something like setFailIfUncommittedChangesOnClose ...

          Show
          shaie Shai Erera added a comment - I don't think this patch is correct. IndexWriter uses Version not in order to commitOnClose, but to throw an exception if you have uncommitted changes. You see, IndexWriter.close() no longer commits in trunk, it always discards uncommitted changes. The only difference is whether they are discarded silently or loudly. That's why I said that this code should be either removed or deprecated in 5.0 and removed in 6.0. The only reason this code exists is that we want people who migrate from 4.x to 5.0 will not silently break their app if they didn't call commit() prior to close() (or called shutdown()). But I'm not sure these users will know to pass Version < 5.0 in order to set that behavior, unless they already do. Anyway, if we want to make this patch valid, we should name the setter something like setFailIfUncommittedChangesOnClose ...
          Hide
          mikemccand Michael McCandless added a comment -

          Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close").

          And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false).

          If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost.

          Show
          mikemccand Michael McCandless added a comment - Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close"). And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false). If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost.
          Hide
          shaie Shai Erera added a comment -

          We could change the semantics, but I pointed out that this patch doesn't in fact do that, except maybe it intends to do it .

          But won't changing the semantics to commit + waitMerges by default sort of rollback the change we did on LUCENE-4246? And if we do that, why do we need shutdown()? I mean, if I can set IWC.commitOnClose(true), should I also call shutdown?

          Show
          shaie Shai Erera added a comment - We could change the semantics, but I pointed out that this patch doesn't in fact do that, except maybe it intends to do it . But won't changing the semantics to commit + waitMerges by default sort of rollback the change we did on LUCENE-4246 ? And if we do that, why do we need shutdown()? I mean, if I can set IWC.commitOnClose(true), should I also call shutdown?
          Hide
          shaie Shai Erera added a comment -

          I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). That will at least remove a slew of warnings in eclipse that IndexWriter is an unclosed resource .

          Show
          shaie Shai Erera added a comment - I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). That will at least remove a slew of warnings in eclipse that IndexWriter is an unclosed resource .
          Hide
          rcmuir Robert Muir added a comment -

          Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close").

          And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false).

          If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost

          +1

          This gives the option for advanced users but prevents any scary mailing lists messages about people losing documents because they passed VERSION_CURRENT.

          Show
          rcmuir Robert Muir added a comment - Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close"). And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false). If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost +1 This gives the option for advanced users but prevents any scary mailing lists messages about people losing documents because they passed VERSION_CURRENT.
          Hide
          mikemccand Michael McCandless added a comment -

          I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close().

          +1

          Show
          mikemccand Michael McCandless added a comment - I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). +1
          Hide
          rjernst Ryan Ernst added a comment -

          Thanks for all the ideas! This new patch removes shutdown() (as a public method), and makes close() act based on commitOnClose. There are a couple things still to fix:

          • I'm unsure if TestIndexWriter.testCloseWhileMergeIsRunning is really doing anything (nothing waits on mergeStarted? and the exc check of the old behavior was the only assertion?)
          • TestIndexWriterMerging.testNoWaitClose hangs (I haven't looked at this at all yet)
          • I haven't checked the latest changes against Solr yet.
          Show
          rjernst Ryan Ernst added a comment - Thanks for all the ideas! This new patch removes shutdown() (as a public method), and makes close() act based on commitOnClose . There are a couple things still to fix: I'm unsure if TestIndexWriter.testCloseWhileMergeIsRunning is really doing anything (nothing waits on mergeStarted? and the exc check of the old behavior was the only assertion?) TestIndexWriterMerging.testNoWaitClose hangs (I haven't looked at this at all yet) I haven't checked the latest changes against Solr yet.
          Hide
          mikemccand Michael McCandless added a comment -

          Hmm, patch doesn't quite cleanly apply on trunk?

          Show
          mikemccand Michael McCandless added a comment - Hmm, patch doesn't quite cleanly apply on trunk?
          Hide
          mikemccand Michael McCandless added a comment -

          Oh, nevermind, I see: the 2nd patch isn't cumulative ... once I applied 1st patch, 2nd patch applied fine.

          Show
          mikemccand Michael McCandless added a comment - Oh, nevermind, I see: the 2nd patch isn't cumulative ... once I applied 1st patch, 2nd patch applied fine.
          Hide
          mikemccand Michael McCandless added a comment -

          Here's a new patch w/ just a couple changes over the first 2:

          • Default commitOnClose to true
          • Fixed test failures in TestIndexWriterWithThreads

          Lucene tests seem to pass with this patch ...

          Show
          mikemccand Michael McCandless added a comment - Here's a new patch w/ just a couple changes over the first 2: Default commitOnClose to true Fixed test failures in TestIndexWriterWithThreads Lucene tests seem to pass with this patch ...
          Hide
          mikemccand Michael McCandless added a comment -

          Woops, I forgot one more change:

          • I fixed IW.close, in the commitOnClose=true case, to not call shutdown if the IW is already closed.
          Show
          mikemccand Michael McCandless added a comment - Woops, I forgot one more change: I fixed IW.close, in the commitOnClose=true case, to not call shutdown if the IW is already closed.
          Hide
          rjernst Ryan Ernst added a comment -

          Here's another patch that just fixes one test (I don't know why it didn't show up as a compile error before, it may have been a result of a recent sync up with trunk).

          I am still getting a hang with TestIndexWriterMerging.testNoWaitClose, but only when I run ant test within lucene/core, not when I run that test individually (or the suite).

          Show
          rjernst Ryan Ernst added a comment - Here's another patch that just fixes one test (I don't know why it didn't show up as a compile error before, it may have been a result of a recent sync up with trunk). I am still getting a hang with TestIndexWriterMerging.testNoWaitClose, but only when I run ant test within lucene/core, not when I run that test individually (or the suite).
          Hide
          rjernst Ryan Ernst added a comment -

          Ok, another patch with the sometimes failing tests now passing (Mike beasted many iterations). Solr tests also pass.

          I'll commit Friday afternoon PST if there are no objections.

          Show
          rjernst Ryan Ernst added a comment - Ok, another patch with the sometimes failing tests now passing (Mike beasted many iterations). Solr tests also pass. I'll commit Friday afternoon PST if there are no objections.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1617004 from Ryan Ernst in branch 'dev/trunk'
          [ https://svn.apache.org/r1617004 ]

          LUCENE-5871: Remove Version from IndexWriterConfig

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1617004 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1617004 ] LUCENE-5871 : Remove Version from IndexWriterConfig
          Hide
          mikemccand Michael McCandless added a comment -

          I'd like to backport the improvement to IW.close to 4.x, even though the rest of this is 5.0 ... that improvement makes IW.close's behavior on exception sane (it's actually closed)...

          Show
          mikemccand Michael McCandless added a comment - I'd like to backport the improvement to IW.close to 4.x, even though the rest of this is 5.0 ... that improvement makes IW.close's behavior on exception sane (it's actually closed)...
          Hide
          mikemccand Michael McCandless added a comment -

          Here's a patch for fixing IW.close on 4.x ... I also fixed trunk's close a bit (put back the shouldClose() call), and added a couple test cases.

          For 4.x, I also back-ported a bug fix + test that snuck in with LUCENE-4246.

          I think it's ready...

          Show
          mikemccand Michael McCandless added a comment - Here's a patch for fixing IW.close on 4.x ... I also fixed trunk's close a bit (put back the shouldClose() call), and added a couple test cases. For 4.x, I also back-ported a bug fix + test that snuck in with LUCENE-4246 . I think it's ready...
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          LUCENE-5871: one one thread can IW.close at once

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1617961 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1617961 ] LUCENE-5871 : one one thread can IW.close at once
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          LUCENE-5871: fix silly test bug

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1617991 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1617991 ] LUCENE-5871 : fix silly test bug
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          LUCENE-5871: IW.close really closes the IndexWriter, even on exception

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1618062 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1618062 ] LUCENE-5871 : IW.close really closes the IndexWriter, even on exception
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          LUCENE-5871: test bugs

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1618063 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1618063 ] LUCENE-5871 : test bugs
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1619623 from Use account "steve_rowe" instead in branch 'dev/trunk'
          [ https://svn.apache.org/r1619623 ]

          LUCENE-5859, LUCENE-5871: Remove Version.LUCENE_CURRENT from javadocs

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1619623 from Use account "steve_rowe" instead in branch 'dev/trunk' [ https://svn.apache.org/r1619623 ] LUCENE-5859 , LUCENE-5871 : Remove Version.LUCENE_CURRENT from javadocs
          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:
              rjernst Ryan Ernst
              Reporter:
              rjernst Ryan Ernst
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development