Jackrabbit Oak
  1. Jackrabbit Oak
  2. OAK-571

Limit usage of headRevId in sync collection to non-branches

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.6
    • Component/s: mongomk
    • Labels:
      None

      Description

      Currently the head revision in the sync collection is also updated for branch commits. This makes some commands more complicated (e.g. FetchHeadRevisionIdAction must check for branch commits). With the current implementation it may also happen that a commit is exposed, which is later marked as failed. This happens when the head revision is set to a higher value (because of a branch commit) than the current head revision of the trunk. Now it may happen that a commit with revision R to trunk with head-of-trunk < R < head-of-branch is exposed as valid commit for some time but may later marked failed.

      1. OAK-571.patch
        2 kB
        Mete Atamel
      2. OAK-571-2.patch
        2 kB
        Mete Atamel
      3. OAK-571-3.patch
        4 kB
        Marcel Reutegger
      4. OAK-571-4.patch
        7 kB
        Mete Atamel

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          1d 18h 56m 1 Mete Atamel 23/Jan/13 07:32
          Resolved Resolved Closed Closed
          103d 8h 16m 1 Jukka Zitting 06/May/13 16:49
          Jukka Zitting made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Jukka Zitting made changes -
          Fix Version/s 0.6 [ 12323297 ]
          Hide
          Marcel Reutegger added a comment -

          With the current implementation it may also happen that a commit is exposed, which is later marked as failed.

          Unfortunately this can still happen, even with readRevId only updated for trunk.

          MongoMKBranchMergeTest#concurrentNonConflictingMerges doesn't expose this problem because the changes are non conflicting. The test only exercises concurrency.

          I think the test in OAK-566 shows what happens when commits are concurrent and conflicting (to some degree).

          I still think it was a good move to limit the usage of headRevId because it makes the implementation simpler and should have better concurrency with branch commit (no need to update headRevId in this case). We just have to keep in mind the problem stated in the description of this issue is not yet completely solved. I'd say we can follow up in OAK-566.

          Show
          Marcel Reutegger added a comment - With the current implementation it may also happen that a commit is exposed, which is later marked as failed. Unfortunately this can still happen, even with readRevId only updated for trunk. MongoMKBranchMergeTest#concurrentNonConflictingMerges doesn't expose this problem because the changes are non conflicting. The test only exercises concurrency. I think the test in OAK-566 shows what happens when commits are concurrent and conflicting (to some degree). I still think it was a good move to limit the usage of headRevId because it makes the implementation simpler and should have better concurrency with branch commit (no need to update headRevId in this case). We just have to keep in mind the problem stated in the description of this issue is not yet completely solved. I'd say we can follow up in OAK-566 .
          Mete Atamel made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Mete Atamel added a comment -

          Resolved with rev 1437278.

          Show
          Mete Atamel added a comment - Resolved with rev 1437278.
          Hide
          Marcel Reutegger added a comment -

          Shall I go ahead and check this in?

          Yes, please. Looks good to me.

          Show
          Marcel Reutegger added a comment - Shall I go ahead and check this in? Yes, please. Looks good to me.
          Mete Atamel made changes -
          Attachment OAK-571-4.patch [ 12565952 ]
          Hide
          Mete Atamel added a comment -

          Minor tweak to the previous patch to include CommitCommand in the fix.

          Show
          Mete Atamel added a comment - Minor tweak to the previous patch to include CommitCommand in the fix.
          Hide
          Mete Atamel added a comment -

          Same here, I ran concurrentNonConflictingMerges a number of times as it is and also a number of times with more threads and it works with your FetchCommitsAction change. Shall I go ahead and check this in?

          Show
          Mete Atamel added a comment - Same here, I ran concurrentNonConflictingMerges a number of times as it is and also a number of times with more threads and it works with your FetchCommitsAction change. Shall I go ahead and check this in?
          Marcel Reutegger made changes -
          Attachment OAK-571-3.patch [ 12565947 ]
          Hide
          Marcel Reutegger added a comment -

          Attached patch simply returns all commits fetched from MongoDB. oak-mongomk tests succeed.

          I haven't seen any concurrentNonConflictingMerges() failures even after running it multiple times.

          Show
          Marcel Reutegger added a comment - Attached patch simply returns all commits fetched from MongoDB. oak-mongomk tests succeed. I haven't seen any concurrentNonConflictingMerges() failures even after running it multiple times.
          Hide
          Mete Atamel added a comment -

          Yep, I found the same thing. This was designed by Philipp but AFAIU by having a baseRevId/revId, one could have any value for revision id (eg. non numeric, non increasing) and still traverse through the commits by following the baseRevId chain.

          Show
          Mete Atamel added a comment - Yep, I found the same thing. This was designed by Philipp but AFAIU by having a baseRevId/revId, one could have any value for revision id (eg. non numeric, non increasing) and still traverse through the commits by following the baseRevId chain.
          Hide
          Marcel Reutegger added a comment -

          It doesn't work because of FetchCommitsAction.convertToCommits().

          IIUC it starts with the highest revision commit and collects the commits along the base revision chain. It may happen that a branch commit is not considered, because it is not referenced by any other commit.

          What I don't understand is, why it does the filtering. Why doesn't it just return all commits it got from the cursor?

          Show
          Marcel Reutegger added a comment - It doesn't work because of FetchCommitsAction.convertToCommits(). IIUC it starts with the highest revision commit and collects the commits along the base revision chain. It may happen that a branch commit is not considered, because it is not referenced by any other commit. What I don't understand is, why it does the filtering. Why doesn't it just return all commits it got from the cursor?
          Hide
          Mete Atamel added a comment -

          Right. There's also another somewhat related issue that I opened OAK-577 for.

          Show
          Mete Atamel added a comment - Right. There's also another somewhat related issue that I opened OAK-577 for.
          Hide
          Marcel Reutegger added a comment -

          Hmm, while running concurrentNonConflictingMerges I sometimes see getNonConflictingCommitsDiff() return null. This shouldn't happen, because commits are non-conflicting.

          Show
          Marcel Reutegger added a comment - Hmm, while running concurrentNonConflictingMerges I sometimes see getNonConflictingCommitsDiff() return null. This shouldn't happen, because commits are non-conflicting.
          Hide
          Marcel Reutegger added a comment -

          I agree, there's something strange or wrong here. The commits are non-conflicting and there's no need to include anything from another merge.

          Show
          Marcel Reutegger added a comment - I agree, there's something strange or wrong here. The commits are non-conflicting and there's no need to include anything from another merge.
          Hide
          Mete Atamel added a comment -

          Not totally sure but I think there's something wrong with MergeCommand. Here's an example of a conflicting commit that I get in concurrentNonConflictingMerges with the latest patch I attached:

          Conflict @597 with @593
          Diff@597: +"/test/t1/node9":{}
          Diff@593: "/test/t1/node9":{}"/test/t0/node10":{}

          As seen, rev 593 has an extra diff but concurrentNonConflictingMerges branches and merges in every commit, so I'm not sure if multiple diffs are expected in this case. One other problem is MergeCommand commits only once, so if there's a conflicting commit, there's no retry. I can fix that by calling MongoNodeStore#commit instead and that does retries and fixes the test but I think this will just hide the underlying problem with MergeCommand.

          Show
          Mete Atamel added a comment - Not totally sure but I think there's something wrong with MergeCommand. Here's an example of a conflicting commit that I get in concurrentNonConflictingMerges with the latest patch I attached: Conflict @597 with @593 Diff@597: +"/test/t1/node9":{} Diff@593: "/test/t1/node9":{} "/test/t0/node10":{} As seen, rev 593 has an extra diff but concurrentNonConflictingMerges branches and merges in every commit, so I'm not sure if multiple diffs are expected in this case. One other problem is MergeCommand commits only once, so if there's a conflicting commit, there's no retry. I can fix that by calling MongoNodeStore#commit instead and that does retries and fixes the test but I think this will just hide the underlying problem with MergeCommand.
          Mete Atamel made changes -
          Attachment OAK-571-2.patch [ 12565920 ]
          Hide
          Mete Atamel added a comment -

          This second patch seems to work better. concurrentNonConflictingMerges still fails but this time it does not fail with nonexisting commit but rather fails with a conflicting commit.

          Show
          Mete Atamel added a comment - This second patch seems to work better. concurrentNonConflictingMerges still fails but this time it does not fail with nonexisting commit but rather fails with a conflicting commit.
          Mete Atamel made changes -
          Attachment OAK-571.patch [ 12565771 ]
          Hide
          Mete Atamel added a comment -

          Attached is a partial solution. All the tests except MongoMKBranchMergeTest#concurrentNonConflictingMerges pass. That test fails because CommitCommandNew does not handle concurrent conflicting commit detection correctly anymore with the changes.

          Show
          Mete Atamel added a comment - Attached is a partial solution. All the tests except MongoMKBranchMergeTest#concurrentNonConflictingMerges pass. That test fails because CommitCommandNew does not handle concurrent conflicting commit detection correctly anymore with the changes.
          Hide
          Marcel Reutegger added a comment -

          Blocks OAK-560.

          Show
          Marcel Reutegger added a comment - Blocks OAK-560 .
          Marcel Reutegger made changes -
          Field Original Value New Value
          Link This issue blocks OAK-560 [ OAK-560 ]
          Marcel Reutegger created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Marcel Reutegger
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development