-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-1909. Use new HA code for Non-HA in OM. #1225
Conversation
76af6ab
to
6169be1
Compare
c18a1e0
to
5ebb33f
Compare
/retest |
/retest |
338776d
to
a425167
Compare
/retest |
...r/src/main/java/org/apache/hadoop/ozone/om/request/security/OMGetDelegationTokenRequest.java
Show resolved
Hide resolved
...main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java
Outdated
Show resolved
Hide resolved
omClientResponse.getFlushFuture().get(); | ||
LOG.trace("Future for {} is completed", request); | ||
} catch (ExecutionException | InterruptedException ex) { | ||
// Do we need to terminate OM here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only potential issue is if we update the cache but fail to update RocksDB. In that case self-terminate is likely the best option. Since Ratis is not enabled the request is not persistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, terminate OM here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would err on the side of safety and self-terminate the OM here.
...main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestS3BucketManager.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestS3BucketManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more comments.
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestS3BucketManager.java
Show resolved
Hide resolved
...r/src/test/java/org/apache/hadoop/ozone/om/request/volume/acl/TestOMVolumeAddAclRequest.java
Show resolved
Hide resolved
...r/src/test/java/org/apache/hadoop/ozone/om/request/volume/acl/TestOMVolumeSetAclRequest.java
Show resolved
Hide resolved
4db799d
to
476407f
Compare
Thank You @arp7 for the review. |
/retest |
476407f
to
9a74db9
Compare
Fixed s3 test failure. |
/retest |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Thanks @bharatviswa504. The patch looks pretty good to me. +1 One remaining point was to self-terminate the OM in the RocksDB update failure path. It can be done in a separate jira. |
+1 to commit assuming the integration test failures are unrelated. |
Thank You @arp7 for the review. |
Opened jira's HDDS-2078 and HDDS-2079 for secure cluster test failures. |
No description provided.