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

        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