Solr
  1. Solr
  2. SOLR-7659

DirectoryFileStream.releaseCommitPointAndExtendReserve -- should it extend first?

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.2.1
    • Fix Version/s: 5.3, 6.0
    • Component/s: replication (java)
    • Labels:
      None

      Description

      After a misunderstanding on my part, leading to an incorrect documentation edit, Shalin Shekhar Mangar helped me understand how the commit reserve duration works.

      The resulting discussion on the dev list brought up a possible problem in ReplicationHandler. We wondered whether it might be possible for another thread to sneak in a commit point deletion in between the two statements found in the releaseCommitPointAndExtendReserve method on the DirectoryFileStream object. If it is safe to do so, I propose reversing those two statements so the reserve extension is done before releasing the commit point.

      1. SOLR-7659.patch
        2 kB
        Shawn Heisey
      2. SOLR-7659.patch
        3 kB
        Shawn Heisey

        Activity

        Hide
        Shawn Heisey added a comment -

        Patch (against branch_5x) that reverses the statements in the mentioned method. It also renames the method. CHANGES.txt mentions this in the "upgrading" section.

        Show
        Shawn Heisey added a comment - Patch (against branch_5x) that reverses the statements in the mentioned method. It also renames the method. CHANGES.txt mentions this in the "upgrading" section.
        Hide
        Shawn Heisey added a comment - - edited

        It's not completely necessary to rename the method, but it seemed like a good thing to do based on changing what the code does. Within the Solr source, it is only used locally, and the number of users that are likely to write code extending that particular inner class should be very low. Also, the inner class is private.

        Show
        Shawn Heisey added a comment - - edited It's not completely necessary to rename the method, but it seemed like a good thing to do based on changing what the code does. Within the Solr source, it is only used locally, and the number of users that are likely to write code extending that particular inner class should be very low. Also, the inner class is private.
        Hide
        Shawn Heisey added a comment -

        I should know this, but I'm not sure ... since that inner class is private, do we even need to mention implementation changes?

        Show
        Shawn Heisey added a comment - I should know this, but I'm not sure ... since that inner class is private, do we even need to mention implementation changes?
        Hide
        Shalin Shekhar Mangar added a comment -

        since that inner class is private, do we even need to mention implementation changes?

        No, calling out the method name change in CHANGES.txt is not necessary. Nobody else can really use or modify this method because, as you noted, the class is inner and private.

        Patch looks good. Commit away!

        Show
        Shalin Shekhar Mangar added a comment - since that inner class is private, do we even need to mention implementation changes? No, calling out the method name change in CHANGES.txt is not necessary. Nobody else can really use or modify this method because, as you noted, the class is inner and private. Patch looks good. Commit away!
        Hide
        Shawn Heisey added a comment -

        Adjusted patch. Solr tests passed with this applied. That's not a full indication that there are no problems, but it's very encouraging.

        Show
        Shawn Heisey added a comment - Adjusted patch. Solr tests passed with this applied. That's not a full indication that there are no problems, but it's very encouraging.
        Hide
        ASF subversion and git services added a comment -

        Commit 1684808 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1684808 ]

        SOLR-7659: Rename and reorganize DirectoryFileStream.releaseCommitPointAndExtendReserve.

        Show
        ASF subversion and git services added a comment - Commit 1684808 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1684808 ] SOLR-7659 : Rename and reorganize DirectoryFileStream.releaseCommitPointAndExtendReserve.
        Hide
        Shawn Heisey added a comment -

        Tests and precommit passed before trunk commit.

        Show
        Shawn Heisey added a comment - Tests and precommit passed before trunk commit.
        Hide
        ASF subversion and git services added a comment -

        Commit 1684811 from Shawn Heisey in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1684811 ]

        SOLR-7659: Rename and reorganize DirectoryFileStream.releaseCommitPointAndExtendReserve. (merge trunk r1684808)

        Show
        ASF subversion and git services added a comment - Commit 1684811 from Shawn Heisey in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1684811 ] SOLR-7659 : Rename and reorganize DirectoryFileStream.releaseCommitPointAndExtendReserve. (merge trunk r1684808)
        Hide
        Shawn Heisey added a comment -

        Precommit passed on branch_5x before commit. When I switched JAVA_HOME to Oracle jdk7u80, solr tests passed, but with Oracle jdk8u45, a very large number of tests were failing.

        Show
        Shawn Heisey added a comment - Precommit passed on branch_5x before commit. When I switched JAVA_HOME to Oracle jdk7u80, solr tests passed, but with Oracle jdk8u45, a very large number of tests were failing.
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk close for 5.3.0 release

        Show
        Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

          People

          • Assignee:
            Shawn Heisey
            Reporter:
            Shawn Heisey
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development