HDDS-7279. Snapshot Create requires Double Buffer Flush thread to split the commit batch. by hemantk-12 · Pull Request #3958 · apache/ozone

hemantk-12 · 2022-11-14T19:24:45Z

What changes were proposed in this pull request?

The OmRequest double buffer flush thread flushes the entire buffer as a batch. Since follower OM's will flush batches with different contents, snapshots can't stay consistent between the leader and the followers.
This change is to flush double buffer snapshot aware and flush all the transactions before snapshot create request.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7279

How was this patch tested?

Added unit test.
Tested it by preventing the flush thread from waking up when a key commit request occurs as described by George in jira

…it the commit batch

smengcl · 2022-11-14T19:34:06Z

Hi @GeorgeJahad , if you are interested you can also take a look at this draft as we discussed earlier.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

…notify in OzoneManagerDoubleBuffer

…e changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

…eateSnapshot request as barrier

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

...one-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBuffer.java

…dyFutureQueue before cache cleaning

smengcl · 2022-11-21T19:10:56Z

Looks like there are a bunch of failures in the newly added test:

https://github.com/apache/ozone/actions/runs/3510796319/jobs/5880950816

Error:  Failures: 
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:201 expected: <1> but was: <2>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <3> but was: <4>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <6> but was: <7>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <10> but was: <11>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <14> but was: <15>
[INFO] 
Error:  Tests run: 628, Failures: 5, Errors: 0, Skipped: 1

hemantk-12 · 2022-11-21T19:27:25Z

Looks like there are a bunch of failures in the newly added test:

https://github.com/apache/ozone/actions/runs/3510796319/jobs/5880950816

Error:  Failures: 
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:201 expected: <1> but was: <2>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <3> but was: <4>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <6> but was: <7>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <10> but was: <11>
Error:    TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer:208 expected: <14> but was: <15>
[INFO] 
Error:  Tests run: 628, Failures: 5, Errors: 0, Skipped: 1

Yes, It worked fine on mac but seems like failing due to some race condition in workflow. I thought it will be fixed if we disabled the daemon thread and enable it after adding responses to the currentBuffer. But still failing.

I'm gone extract out the flushing logic and test it independently.

smengcl

Thanks @hemanthboyina for the patch. Overall looks good. I have some minor comments inline.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

smengcl · 2022-11-22T01:40:35Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

+      // New queue gets created in three conditions:
+      // 1. It is first element in the response,
+      // 2. Current request is createSnapshot request.
+      // 3. Previous request was createSnapshot request.
+      if (response.isEmpty() ||
+          omResponse.getCreateSnapshotResponse() != null ||
+          (previousOmResponse != null &&
+              previousOmResponse.getCreateSnapshotResponse() != null)) {


The logic sounds good to me. But IntelliJ somehow gives scary "condition always true" warning and suggests removal of the condition. I think this is a false alarm. Would you also double check?

Yes, I think it is because CreateSnapshotResponse is optional attribute.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java

prashantpogde

Thanks for the patch @hemantk-12. LGTM

smengcl · 2022-11-28T21:14:38Z

Thanks @hemantk-12 for the patch, @GeorgeJahad and @prashantpogde for reviewing this.

HDDS-7279. Snapshot Create requires Double Buffer Flush thread to spl…

908f185

…it the commit batch

hemantk-12 commented Nov 15, 2022

View reviewed changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java Outdated Show resolved Hide resolved

hemantk-12 added 2 commits November 14, 2022 18:27

Use batch buffer instead of ready buffer and removed if check before …

f4c89dd

…notify in OzoneManagerDoubleBuffer

HDDS-7279. Snapshot: Added unit tests for double buffer snapshot awar…

7829f74

…e changes

hemantk-12 changed the title ~~HDDS-7279. Snapshot Create requires Double Buffer Flush thread to spl…~~ HDDS-7279. Snapshot Create requires Double Buffer Flush thread to split the commit batch. Nov 16, 2022

prashantpogde reviewed Nov 16, 2022

View reviewed changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java Outdated Show resolved Hide resolved

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java Outdated Show resolved Hide resolved

HDDS-7279. Snapshot: Change the readyBuffer splitting logic to use cr…

e6af86e

…eateSnapshot request as barrier

hemantk-12 marked this pull request as ready for review November 16, 2022 21:02

hemantk-12 requested a review from prashantpogde November 16, 2022 21:03

hemantk-12 added 4 commits November 16, 2022 14:05

HDDS-7279. Snapshot: Fixed CI failed xchecks

9e9d7b7

HDDS-7279. Snapshot: Fixed Checkstyle failed checks

2b85438

HDDS-7279. Snapshot: Fixed findbug check failure

3de8d58

HDDS-7279. Snapshot: Fixed the findbugs and unit test failure

fcdcb61

GeorgeJahad reviewed Nov 19, 2022

View reviewed changes

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java Show resolved Hide resolved

GeorgeJahad reviewed Nov 19, 2022

View reviewed changes

...one-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBuffer.java Outdated Show resolved Hide resolved

GeorgeJahad reviewed Nov 19, 2022

View reviewed changes

...one-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBuffer.java Show resolved Hide resolved

HDDS-7279. Snapshot: Updated test with comment and moved cleaning Rea…

9719247

…dyFutureQueue before cache cleaning

kerneltime added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Nov 21, 2022

Fixed the unit test failure

87c5766

smengcl reviewed Nov 22, 2022

View reviewed changes

Addressed review comments

3bf3b50

smengcl approved these changes Nov 23, 2022

View reviewed changes

prashantpogde approved these changes Nov 28, 2022

View reviewed changes

smengcl merged commit 6fe2ab8 into apache:HDDS-6517-Snapshot Nov 28, 2022

hemantk-12 deleted the HDDS-7279 branch December 8, 2022 18:55

Conversation

hemantk-12 commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

smengcl commented Nov 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smengcl commented Nov 21, 2022

Uh oh!

hemantk-12 commented Nov 21, 2022

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smengcl Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prashantpogde left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hemantk-12 commented Nov 14, 2022 •

edited

Loading