Lucene - Core
  1. Lucene - Core
  2. LUCENE-5925

Use rename instead of segments_N fallback / segments.gen etc

    Details

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

      Description

      Our commit logic is strange, we write corrupted commit points and only on the last phase of commit do we "correct them".

      This means the logic to get the latest commit is always scary and awkward, since it must deal with partial commits, and try to determine if it should fall back to segments_N-1 or actually relay an exception.

      This logic is incomplete/sheisty as we, e.g. i think we only fall back so far (at most one).

      If we somehow screw up in all this logic do the wrong thing, then we lose data (e.g. LUCENE-4870 wiped entire index because of TooManyOpenFiles).

      We now require java 7, i think we should expore instead writing pending_segments_N and then in finishCommit() doing an atomic rename to segments_N.

      We could then remove all the complex fallback logic completely, since we no longer have to deal with "ignoring partial commits", instead simply delivering any exception we get when trying to read the commit and sleep better at night.

      In java 7, we have the apis for this (ATOMIC_MOVE).

      1. LUCENE-5925.patch
        66 kB
        Robert Muir
      2. LUCENE-5925.patch
        65 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Michael McCandless added a comment -

          +1, this would be so much simpler, and it's great Java 7 gives us the APIs to do this.

          Show
          Michael McCandless added a comment - +1, this would be so much simpler, and it's great Java 7 gives us the APIs to do this.
          Hide
          Yonik Seeley added a comment -

          Do we still need to contend with Windows and the possibility that a virus scanner or something else peeks at pending_segments_N at the wrong time when we are trying to move it? Does that cause the ATOMIC_MOVE to fail?

          Show
          Yonik Seeley added a comment - Do we still need to contend with Windows and the possibility that a virus scanner or something else peeks at pending_segments_N at the wrong time when we are trying to move it? Does that cause the ATOMIC_MOVE to fail?
          Hide
          Robert Muir added a comment -
          Show
          Robert Muir added a comment - yes. the main thing it doesnt guarantee is atomic replacement, but we don't need that: http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#move%28java.nio.file.Path,%20java.nio.file.Path,%20java.nio.file.CopyOption...%29
          Hide
          Uwe Schindler added a comment -

          Does that cause the ATOMIC_MOVE to fail?

          Yes it could in windows. But this would not be bad, it just fails the commit. You get Exception and all is fine, no corrumption.
          As we never move to an already existing file or different file system, atomic moves should always work.

          Show
          Uwe Schindler added a comment - Does that cause the ATOMIC_MOVE to fail? Yes it could in windows. But this would not be bad, it just fails the commit. You get Exception and all is fine, no corrumption. As we never move to an already existing file or different file system, atomic moves should always work.
          Hide
          Shai Erera added a comment -

          What would be the fallback logic in case of AtomicMoveNotSupportedException? E.g. if it's a shared FS, or distributed FS like GPFS (which is usable with FSDirectory)?

          Show
          Shai Erera added a comment - What would be the fallback logic in case of AtomicMoveNotSupportedException ? E.g. if it's a shared FS, or distributed FS like GPFS (which is usable with FSDirectory)?
          Hide
          Robert Muir added a comment -

          You would get an exception. So it needs to be supported.

          Show
          Robert Muir added a comment - You would get an exception. So it needs to be supported.
          Hide
          Robert Muir added a comment -

          By the way, like the java apis, we can relax the requirements on the Directory method:

          • guarantee target will never exist
          • we don't care about atomicity of "source" vs "dest", its fine if both exist for a while, etc.
          • don't require that "dest" is visible in listAll() on return, as long as fsyncing the directory will make it so.
          • ...

          All we care about is the contents of "dest" appear atomic for read operations against it.

          Show
          Robert Muir added a comment - By the way, like the java apis, we can relax the requirements on the Directory method: guarantee target will never exist we don't care about atomicity of "source" vs "dest", its fine if both exist for a while, etc. don't require that "dest" is visible in listAll() on return, as long as fsyncing the directory will make it so. ... All we care about is the contents of "dest" appear atomic for read operations against it.
          Hide
          Shai Erera added a comment -

          You would get an exception. So it needs to be supported.

          What do you mean? We will no longer support indexing on FileSystems that don't support atomic-move?

          Show
          Shai Erera added a comment - You would get an exception. So it needs to be supported. What do you mean? We will no longer support indexing on FileSystems that don't support atomic-move?
          Hide
          Robert Muir added a comment -

          Exactly, it would be an abstract method in the Directory API with the semantics i described above.

          Show
          Robert Muir added a comment - Exactly, it would be an abstract method in the Directory API with the semantics i described above.
          Hide
          Robert Muir added a comment -

          Initial patch. all tests pass.

          Show
          Robert Muir added a comment - Initial patch. all tests pass.
          Hide
          Uwe Schindler added a comment -

          Hi,
          I like the patch. The description of the rename operation is good: The renamed file must appear atomically, not the rename itsself.

          I can beast the whole stuff on Windows later!

          Show
          Uwe Schindler added a comment - Hi, I like the patch. The description of the rename operation is good: The renamed file must appear atomically, not the rename itsself. I can beast the whole stuff on Windows later!
          Hide
          Uwe Schindler added a comment - - edited

          I ran ant beast -Dbeast.iters=10 -Dtests.dups=6 -Dtestcase=TestIndex* -Dtests.directory=SimpleFSDirectory on Windows 7 without errors. Took more than an hour!

          Show
          Uwe Schindler added a comment - - edited I ran ant beast -Dbeast.iters=10 -Dtests.dups=6 -Dtestcase=TestIndex* -Dtests.directory=SimpleFSDirectory on Windows 7 without errors. Took more than an hour!
          Hide
          Uwe Schindler added a comment -

          The full test suite (Lucene + Solr) also succeeded on Windows.

          Show
          Uwe Schindler added a comment - The full test suite (Lucene + Solr) also succeeded on Windows.
          Hide
          Michael McCandless added a comment -

          +1, patch looks great, and I ran 56 iterations of full Lucene core + module tests w/ distributed beaster and no failures ...

          Show
          Michael McCandless added a comment - +1, patch looks great, and I ran 56 iterations of full Lucene core + module tests w/ distributed beaster and no failures ...
          Hide
          Robert Muir added a comment -

          Updated patch. This also solves LUCENE-2585.

          Today, segments.gen "helps" with the fact that directory listing is not point in time (its a weakly consistent iterator and can reflect changes that happen during iteration). But it cannot be totally relied upon due to timing (you can get unlucky like LUCENE-2585).

          Instead, in FindSegmentsFile, we simply detect that contents have changed during the execution of listAll, by executing it again and doing a comparison. This way we can detect "ConcurrentModificationException" and just continue the loop.

          Show
          Robert Muir added a comment - Updated patch. This also solves LUCENE-2585 . Today, segments.gen "helps" with the fact that directory listing is not point in time (its a weakly consistent iterator and can reflect changes that happen during iteration). But it cannot be totally relied upon due to timing (you can get unlucky like LUCENE-2585 ). Instead, in FindSegmentsFile, we simply detect that contents have changed during the execution of listAll, by executing it again and doing a comparison. This way we can detect "ConcurrentModificationException" and just continue the loop.
          Hide
          Michael McCandless added a comment -

          +1, I like this solution. It passed 85 iters of all lucene core + module tests...

          Show
          Michael McCandless added a comment - +1, I like this solution. It passed 85 iters of all lucene core + module tests...
          Hide
          Robert Muir added a comment -

          Thanks for beasting guys! I ran 150 iterations of 'test-core' myself. I'll give it a few days.

          Show
          Robert Muir added a comment - Thanks for beasting guys! I ran 150 iterations of 'test-core' myself. I'll give it a few days.
          Hide
          ASF subversion and git services added a comment -

          Commit 1624194 from Robert Muir in branch 'dev/trunk'
          [ https://svn.apache.org/r1624194 ]

          LUCENE-5925: use rename instead of segments_N fallback/segments.gen

          Show
          ASF subversion and git services added a comment - Commit 1624194 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1624194 ] LUCENE-5925 : use rename instead of segments_N fallback/segments.gen
          Hide
          ASF subversion and git services added a comment -

          Commit 1624362 from Robert Muir in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1624362 ]

          LUCENE-5925: use rename instead of segments_N fallback/segments.gen

          Show
          ASF subversion and git services added a comment - Commit 1624362 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1624362 ] LUCENE-5925 : use rename instead of segments_N fallback/segments.gen
          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:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development