HDDS-1638. Implement Key Write Requests to use Cache and DoubleBuffer. by bharatviswa504 · Pull Request #956 · apache/hadoop

bharatviswa504 · 2019-06-13T01:03:44Z

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.

bharatviswa504 · 2019-06-13T21:21:54Z

Added tests for the classes.

hanishakoneru

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

hanishakoneru · 2019-06-21T01:23:18Z

Precondition checks can be removed from all the KeyRequest classes.

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?

hanishakoneru · 2019-06-21T16:30:02Z

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

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, we will add a isLeader check before calling preExecute. This will help in reducing the chance of allocating blocks on follower OM's.

hanishakoneru · 2019-06-21T16:30:54Z

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

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

hanishakoneru · 2019-06-21T16:37:22Z

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,

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

hanishakoneru

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?

hanishakoneru · 2019-06-21T19:35:55Z

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

hanishakoneru · 2019-06-21T19:58:51Z

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

hanishakoneru · 2019-06-21T23:19:12Z

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?

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.

hanishakoneru · 2019-06-21T23:25:57Z

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.

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.

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

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 · 2019-06-24T18:04:16Z

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 · 2019-06-24T18:22:10Z

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

bharatviswa504 · 2019-06-24T20:49:54Z

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

bharatviswa504 · 2019-06-24T21:15:02Z

/retest

bharatviswa504 · 2019-06-26T21:18:28Z

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

hanishakoneru · 2019-06-26T21:25:48Z

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

hadoop-yetus · 2019-06-26T22:55:39Z

💔 -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 · 2019-06-26T23:02:03Z

💔 -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 · 2019-06-27T00:56:08Z

bharatviswa504 · 2019-06-27T00:57:52Z

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 · 2019-06-27T01:22:24Z

I have committed this to trunk.

hadoop-yetus · 2019-06-27T02:41:13Z

💔 -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.

apache#956)

* Flatten startpoint key when serialized. * Provide custom JsonSerializer for StartpointKey.

apache#956)

bharatviswa504 added the ozone label Jun 13, 2019

bharatviswa504 requested review from arp7 and hanishakoneru June 13, 2019 01:03

bharatviswa504 self-assigned this Jun 13, 2019

This comment has been minimized.
Sign in to view

hanishakoneru reviewed Jun 21, 2019
View reviewed changes

bharatviswa504 added 5 commits June 24, 2019 11:13

HDDS-1638. Implement Key Write Requests to use Cache and DoubleBuffer.
6a9591f

add tests.
e07574e

fix checkstyle and test issues.
2faea16

implement isBucketEmpty logic with cache.
5f23e10

add few tests for invalid key name.
56eef3b

bharatviswa504 force-pushed the HDDS-1638 branch from 38c492b to 52e5e08Compare June 24, 2019 18:21

bharatviswa504 force-pushed the HDDS-1638 branch from 52e5e08 to d53616cCompare June 24, 2019 18:52

This comment has been minimized.
Sign in to view

bharatviswa504 force-pushed the HDDS-1638 branch from bd120d5 to 1bd7202Compare June 24, 2019 19:59

This comment has been minimized.
Sign in to view

bharatviswa504 force-pushed the HDDS-1638 branch 2 times, most recently from d12929f to a04a454Compare June 24, 2019 20:48

fix review comments
58a756b

bharatviswa504 force-pushed the HDDS-1638 branch from a04a454 to 58a756bCompare June 24, 2019 21:12

This comment has been minimized.
Sign in to view

remove not required precondition checks in validateAndUpdateCache
8ef5301

This comment has been minimized.
Sign in to view

bharatviswa504 force-pushed the HDDS-1638 branch from 9d569e9 to 284ac9bCompare June 26, 2019 21:23

fix review comment
518b2c3

bharatviswa504 force-pushed the HDDS-1638 branch from 284ac9b to 518b2c3Compare June 26, 2019 21:29

This comment has been minimized.
Sign in to view

fix checkstyle and test
2238e36

bharatviswa504 merged commit 4848280 into apache:trunkJun 27, 2019

bshashikant pushed a commit to bshashikant/hadoop that referenced this pull request Jul 10, 2019
HDDS-1638. Implement Key Write Requests to use Cache and DoubleBuffer. (
3240c75
apache#956)

shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
SAMZA-2132: Flatten startpoint key when serialized. (apache#956)
98355db
* 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
HDDS-1638. Implement Key Write Requests to use Cache and DoubleBuffer. (
e45a10b
apache#956)

Uh oh!

Conversation

bharatviswa504 commented Jun 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bharatviswa504 commented Jun 13, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

hanishakoneru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hanishakoneru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504Jun 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 commented Jun 24, 2019

Uh oh!

bharatviswa504 commented Jun 24, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

bharatviswa504 commented Jun 24, 2019

Uh oh!

bharatviswa504 commented Jun 24, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bharatviswa504 commented Jun 26, 2019

Uh oh!

bharatviswa504 commented Jun 13, 2019•
edited
Loading

bharatviswa504Jun 24, 2019•
edited
Loading