Skip to content
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-1638. Implement Key Write Requests to use Cache and DoubleBuffer. #956

Merged
merged 9 commits into from
Jun 27, 2019

Conversation

bharatviswa504
Copy link
Contributor

@bharatviswa504 bharatviswa504 commented Jun 13, 2019

Implement Key write requests to use OM Cache, double buffer.

In this Jira will add the changes to implement key operations, and HA/Non-HA will have a different code path, but once all requests are implemented will have a single code path.

@hadoop-yetus

This comment has been minimized.

@bharatviswa504
Copy link
Contributor Author

Added tests for the classes.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

Copy link
Contributor

@hanishakoneru hanishakoneru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bharatviswa504 , posting my initial review comments.
I am yet to review some of the KeyRequests and the unit tests.

public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
CommitKeyRequest commitKeyRequest = getOmRequest().getCommitKeyRequest();
Preconditions.checkNotNull(commitKeyRequest);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precondition checks can be removed from all the KeyRequest classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added these checks as these are independent request classes. This will help in detecting errors when some one uses wrongly these classes. Let me know if you still want to remove it?

// bucket/key/volume or not and also with out any authorization checks.
// As for a client for the first time this can be executed on any OM,
// till leader is identified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean any OM can talk to SCM to get blocks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AllocatedBlocks from non leader OM will not be used. Block numbers will be left unused.
Should we add a leader check before allocating blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, we will add a isLeader check before calling preExecute. This will help in reducing the chance of allocating blocks on follower OM's.

// the type and factor read from multipart table, and set the KeyInfo in
// validateAndUpdateCache and return to the client. TODO: See if we can fix
// this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand this. How are we handling multi-part upload?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, can you please add "we do not call allocateBlock in openKey for multipart upload."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

keyName, bucketName, volumeName, ex);
omMetrics.incNumKeyAllocateFails();
auditLog(auditLogger, buildAuditMessage(OMAction.ALLOCATE_KEY, auditMap,
ex, getOmRequest().getUserInfo()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between OMActions ALLOCATE_KEY and CREATE_KEY?
If they represent the same, we should get rid of ALLOCATE_KEY and use the other one instead. We don't have to do that as part of this Jira though,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, we have both ALLOCATE_KEY, CREATE_KEY. I will remove CREATE_KEY as this is not being used anywhere.

Copy link
Contributor

@hanishakoneru hanishakoneru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the ClientRequests and Responses.
Unit tests review is pending.

There is a lot a common code between OMKeyRequest and KeyManagerImpl. Are you planning to clean up KeyManagerImpl in a different Jira?

// the type and factor read from multipart table, and set the KeyInfo in
// validateAndUpdateCache and return to the client. TODO: See if we can fix
// this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, can you please add "we do not call allocateBlock in openKey for multipart upload."

// bucket/key/volume or not and also with out any authorization checks.
// As for a client for the first time this can be executed on any OM,
// till leader is identified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AllocatedBlocks from non leader OM will not be used. Block numbers will be left unused.
Should we add a leader check before allocating blocks?

if (ozoneManager.getAclsEnabled()) {
checkAcls(ozoneManager, OzoneObj.ResourceType.KEY,
OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE,
volumeName, bucketName, fromKeyName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some repetitive code between all the Requests (ACL checks and audit logging). They can probably be combined in one place. We don't need to do this in this Jira. Do you want to open a new Jira for that to keep track?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already we have a method checkAcl in OmRequest class. If we want to reduce the number of lines of code. And remaining lines of code are specific to each request. And same for auditLogging, in super class we have a method, we call that. Let me know if I am missing something here.


// For OmResponse with failure, this should do nothing. This method is
// not called in failure scenario in OM code.
if (getOMResponse().getStatus() == OzoneManagerProtocolProtos.Status.OK) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is performed in all OMClientResponse classes. We can move this check to a calling function.
Again, this can be done in a different Jira if thats preferred.

Copy link
Contributor Author

@bharatviswa504 bharatviswa504 Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a safe guard check. Now each request/response classes are independent, self sufficient classes. If someone in the future directly uses these classes (Don't know if someone will use them) to handle those cases added the check. Right now when OMResponse Status is OK, then only response is added to OM DoubleBuffer in OzoneManagerProtocolServerSideTranslatorPB.java. And this addToDbBatch will be called from OMDoubleBuffer. So, for now, even if someone in future who implements these classes forgot to add this check, we don't see any side affects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In future if some is calling these classes from somewhere else, they should take care of it. We cannot future proof code fully anyway :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone uses these classes and have OMResponse with error, we have guarded them. But someone writes the classes without this check, and then someone uses these classes we shall be in problem. But in our scenario with the current code, it will not be an issue.

On a side note, the reason for this check is when the response is an error, all other parameters are set to null in response classes. This is the main reason for the check.

@bharatviswa504
Copy link
Contributor Author

AllocatedBlocks from non leader OM will not be used. Block numbers will be left unused.
Should we add a leader check before allocating blocks?

As discussed offline, added a check where we call preExecute, so that it applies to all OMRequests.

@bharatviswa504
Copy link
Contributor Author

Thank You @hanishakoneru for the review.
I have addressed the review comments.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@bharatviswa504 bharatviswa504 force-pushed the HDDS-1638 branch 2 times, most recently from d12929f to a04a454 Compare June 24, 2019 20:48
@bharatviswa504
Copy link
Contributor Author

Thank You @hanishakoneru for the review.
I have addressed the review comments.

@bharatviswa504
Copy link
Contributor Author

/retest

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@bharatviswa504
Copy link
Contributor Author

Thank You @hanishakoneru for the review.
Addressed the review comments.

@hanishakoneru
Copy link
Contributor

Thanks for working on this Bharat. +1 pending CI.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 30 Docker mode activated.
_ Prechecks _
+1 dupname 2 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 20 new or modified test files.
_ trunk Compile Tests _
0 mvndep 22 Maven dependency ordering for branch
+1 mvninstall 460 trunk passed
+1 compile 253 trunk passed
+1 checkstyle 64 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 818 branch has no errors when building and testing our client artifacts.
+1 javadoc 157 trunk passed
0 spotbugs 309 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 500 trunk passed
_ Patch Compile Tests _
0 mvndep 35 Maven dependency ordering for patch
+1 mvninstall 433 the patch passed
+1 compile 258 the patch passed
+1 cc 258 the patch passed
+1 javac 258 the patch passed
-0 checkstyle 41 hadoop-ozone: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 690 patch has no errors when building and testing our client artifacts.
+1 javadoc 162 the patch passed
+1 findbugs 518 the patch passed
_ Other Tests _
+1 unit 258 hadoop-hdds in the patch passed.
-1 unit 153 hadoop-ozone in the patch failed.
+1 asflicense 42 The patch does not generate ASF License warnings.
5125
Reason Tests
Failed junit tests hadoop.ozone.om.TestBucketManagerImpl
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-956/15/artifact/out/Dockerfile
GITHUB PR #956
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 79fb7c977b1d 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 1ac967a
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-956/15/artifact/out/diff-checkstyle-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-956/15/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-956/15/testReport/
Max. process+thread count 1375 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/integration-test hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-956/15/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 194 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 20 new or modified test files.
_ trunk Compile Tests _
0 mvndep 104 Maven dependency ordering for branch
+1 mvninstall 671 trunk passed
+1 compile 318 trunk passed
+1 checkstyle 90 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 1110 branch has no errors when building and testing our client artifacts.
+1 javadoc 200 trunk passed
0 spotbugs 381 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 617 trunk passed
_ Patch Compile Tests _
0 mvndep 41 Maven dependency ordering for patch
+1 mvninstall 550 the patch passed
+1 compile 318 the patch passed
+1 cc 318 the patch passed
+1 javac 318 the patch passed
-0 checkstyle 52 hadoop-ozone: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 853 patch has no errors when building and testing our client artifacts.
+1 javadoc 189 the patch passed
-1 findbugs 393 hadoop-ozone generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
_ Other Tests _
-1 unit 224 hadoop-hdds in the patch failed.
-1 unit 247 hadoop-ozone in the patch failed.
+1 asflicense 45 The patch does not generate ASF License warnings.
6720
Reason Tests
FindBugs module:hadoop-ozone
Nullcheck of kv at line 522 of value previously dereferenced in org.apache.hadoop.ozone.om.OmMetadataManagerImpl.isBucketEmpty(String, String) At OmMetadataManagerImpl.java:522 of value previously dereferenced in org.apache.hadoop.ozone.om.OmMetadataManagerImpl.isBucketEmpty(String, String) At OmMetadataManagerImpl.java:[line 522]
Nullcheck of kv at line 480 of value previously dereferenced in org.apache.hadoop.ozone.om.OmMetadataManagerImpl.isVolumeEmpty(String) At OmMetadataManagerImpl.java:480 of value previously dereferenced in org.apache.hadoop.ozone.om.OmMetadataManagerImpl.isVolumeEmpty(String) At OmMetadataManagerImpl.java:[line 480]
Failed junit tests hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerCommandHandler
hadoop.ozone.om.TestS3BucketManager
hadoop.ozone.om.TestBucketManagerImpl
hadoop.ozone.om.request.bucket.TestOMBucketDeleteRequest
hadoop.ozone.om.request.volume.TestOMVolumeDeleteRequest
Subsystem Report/Notes
Docker Client=18.09.5 Server=18.09.5 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/artifact/out/Dockerfile
GITHUB PR #956
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 145002bccfae 4.15.0-48-generic #51-Ubuntu SMP Wed Apr 3 08:28:49 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 1ac967a
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/artifact/out/diff-checkstyle-hadoop-ozone.txt
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/artifact/out/new-findbugs-hadoop-ozone.html
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/testReport/
Max. process+thread count 1338 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/integration-test hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-956/12/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@bharatviswa504
Copy link
Contributor Author


Screen Shot 2019-06-26 at 5 55 48 PM

@bharatviswa504
Copy link
Contributor Author

Thank You @hanishakoneru for the review.
Will fix the checkstyle issue during the commit.

As if I push another commit, we shall lose the CI run output, so attached the CI run screenshot.

@bharatviswa504 bharatviswa504 merged commit 4848280 into apache:trunk Jun 27, 2019
@bharatviswa504
Copy link
Contributor Author

I have committed this to trunk.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 32 Docker mode activated.
_ Prechecks _
+1 dupname 2 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 20 new or modified test files.
_ trunk Compile Tests _
0 mvndep 65 Maven dependency ordering for branch
+1 mvninstall 518 trunk passed
+1 compile 275 trunk passed
+1 checkstyle 66 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 830 branch has no errors when building and testing our client artifacts.
+1 javadoc 159 trunk passed
0 spotbugs 301 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 492 trunk passed
_ Patch Compile Tests _
0 mvndep 34 Maven dependency ordering for patch
+1 mvninstall 456 the patch passed
+1 compile 253 the patch passed
+1 cc 253 the patch passed
+1 javac 253 the patch passed
+1 checkstyle 74 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 1 The patch has no whitespace issues.
+1 shadedclient 642 patch has no errors when building and testing our client artifacts.
-1 javadoc 90 hadoop-ozone generated 2 new + 9 unchanged - 0 fixed = 11 total (was 9)
+1 findbugs 507 the patch passed
_ Other Tests _
+1 unit 240 hadoop-hdds in the patch passed.
-1 unit 1092 hadoop-ozone in the patch failed.
+1 asflicense 41 The patch does not generate ASF License warnings.
6104
Reason Tests
Failed junit tests hadoop.ozone.client.rpc.TestWatchForCommit
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.TestMiniChaosOzoneCluster
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestFailureHandlingByClient
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-956/16/artifact/out/Dockerfile
GITHUB PR #956
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux fe48b0b91314 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 1ac967a
Default Java 1.8.0_212
javadoc https://builds.apache.org/job/hadoop-multibranch/job/PR-956/16/artifact/out/diff-javadoc-javadoc-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-956/16/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-956/16/testReport/
Max. process+thread count 5278 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/integration-test hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-956/16/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

bshashikant pushed a commit to bshashikant/hadoop that referenced this pull request Jul 10, 2019
shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
* Flatten startpoint key when serialized.

* Provide custom JsonSerializer for StartpointKey.
amahussein pushed a commit to amahussein/hadoop that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants