Lucene - Core
  1. Lucene - Core
  2. LUCENE-4973

SnapshotDeletionPolicy should not require an id

    Details

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

      Description

      The id is unnecessary and just adds complexity: SDP can just return
      the IndexCommit, and when you want to release you pass back the
      IndexCommit. PersistentSDP can expose release(long gen).

      1. LUCENE-4973.patch
        43 kB
        Michael McCandless
      2. LUCENE-4973.patch
        42 kB
        Michael McCandless
      3. LUCENE-4973.patch
        40 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch ... SDP is simpler now (I also removed some methods, eg
        getSnapshot, snapshotExists). I think separately we should fix
        PersistentSDP to store its state in a single file ... separate issue.

        Show
        Michael McCandless added a comment - Patch ... SDP is simpler now (I also removed some methods, eg getSnapshot, snapshotExists). I think separately we should fix PersistentSDP to store its state in a single file ... separate issue.
        Hide
        Shai Erera added a comment -

        +1. I will review the patch tomorrow, but I think pSDP needs to have a getIC(gen) because if the apps persists the gens, it needs a way to obtain their IC instance.

        I think separately we should fix PersistentSDP to store its state in a single file ... separate issue.

        +1! would be great if this can be in the index directory, so that app doesn't need to make up locations. but let's discuss that separately.

        Show
        Shai Erera added a comment - +1. I will review the patch tomorrow, but I think pSDP needs to have a getIC(gen) because if the apps persists the gens, it needs a way to obtain their IC instance. I think separately we should fix PersistentSDP to store its state in a single file ... separate issue. +1! would be great if this can be in the index directory, so that app doesn't need to make up locations. but let's discuss that separately.
        Hide
        Michael McCandless added a comment -

        New patch, adding SDP.getIndexCommit(long gen).

        Show
        Michael McCandless added a comment - New patch, adding SDP.getIndexCommit(long gen).
        Hide
        Shai Erera added a comment -

        Looks good! Comments:

        • assertTrue(snapshot == sdp.getIndexCommit(snapshot.getGeneration())); – can you change to assertSame? They should be equivalent, only assertSame produces nicer error message.
          • Same for assertTrue(s1 == s2); // should be the same instance
        • SDP.snapshot javadoc:
          • "using the same ID parameter"; I guess it's a copy-paste error from the previous SDP version?
          • IllegalStateException still refers to ID
        • releaseGen: can we relax it to not fail if someone calls release on an unsnpshotted gen? I guess someone can workaround that by calling sdp.getIC(gen) and if not null call release. But I think it's not necessarily an error to release an unsnapshotted gen, e.g. code which releases from different places. No strong feelings about it though, so it's your call.
        • Persistent – do you want to change SNAPSHOT_ID to SNAPSHOT_GENS?
        Show
        Shai Erera added a comment - Looks good! Comments: assertTrue(snapshot == sdp.getIndexCommit(snapshot.getGeneration())); – can you change to assertSame? They should be equivalent, only assertSame produces nicer error message. Same for assertTrue(s1 == s2); // should be the same instance SDP.snapshot javadoc: "using the same ID parameter"; I guess it's a copy-paste error from the previous SDP version? IllegalStateException still refers to ID releaseGen: can we relax it to not fail if someone calls release on an unsnpshotted gen? I guess someone can workaround that by calling sdp.getIC(gen) and if not null call release. But I think it's not necessarily an error to release an unsnapshotted gen, e.g. code which releases from different places. No strong feelings about it though, so it's your call. Persistent – do you want to change SNAPSHOT_ID to SNAPSHOT_GENS?
        Hide
        Michael McCandless added a comment -

        Thanks Shai! New patch ... I also noticed that onInit (to build the gen -> IC map) was in pSDP but should be in SDP so I moved it up, and I fixed a few other leftover ids in the javadocs ... I think it's ready.

        Show
        Michael McCandless added a comment - Thanks Shai! New patch ... I also noticed that onInit (to build the gen -> IC map) was in pSDP but should be in SDP so I moved it up, and I fixed a few other leftover ids in the javadocs ... I think it's ready.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] mikemccand
        http://svn.apache.org/viewvc?view=revision&revision=1478452

        LUCENE-4973: Persistent/SnapshotDeletionPolicy no longer require a unique id

        Show
        Commit Tag Bot added a comment - [trunk commit] mikemccand http://svn.apache.org/viewvc?view=revision&revision=1478452 LUCENE-4973 : Persistent/SnapshotDeletionPolicy no longer require a unique id
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] mikemccand
        http://svn.apache.org/viewvc?view=revision&revision=1478459

        LUCENE-4973: Persistent/SnapshotDeletionPolicy no longer require a unique id

        Show
        Commit Tag Bot added a comment - [branch_4x commit] mikemccand http://svn.apache.org/viewvc?view=revision&revision=1478459 LUCENE-4973 : Persistent/SnapshotDeletionPolicy no longer require a unique id
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development