HDDS-1205. Refactor ReplicationManager to handle QUASI_CLOSED contain… #620

arp7 · 2019-03-18T20:10:05Z

…ers. Contributed by Nanda kumar.

Change-Id: I578ed5c196bb4d20754607cb9f4ddee7e0599077

hadoop-yetus · 2019-03-18T21:18:46Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	62	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	40	Maven dependency ordering for branch
+1	mvninstall	1247	trunk passed
+1	compile	82	trunk passed
+1	checkstyle	33	trunk passed
+1	mvnsite	76	trunk passed
+1	shadedclient	815	branch has no errors when building and testing our client artifacts.
+1	findbugs	109	trunk passed
+1	javadoc	65	trunk passed
		_ Patch Compile Tests _
0	mvndep	16	Maven dependency ordering for patch
+1	mvninstall	81	the patch passed
+1	compile	72	the patch passed
+1	javac	72	the patch passed
+1	checkstyle	27	the patch passed
+1	mvnsite	64	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	xml	1	The patch has no ill-formed XML file.
+1	shadedclient	802	patch has no errors when building and testing our client artifacts.
+1	findbugs	130	the patch passed
+1	javadoc	55	the patch passed
		_ Other Tests _
+1	unit	77	common in the patch passed.
-1	unit	93	server-scm in the patch failed.
+1	asflicense	25	The patch does not generate ASF License warnings.
		4027

Reason	Tests
Failed junit tests	hadoop.hdds.scm.node.TestSCMNodeManager

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-620/1/artifact/out/Dockerfile
GITHUB PR	#620
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
uname	Linux 17db0b7b30ca 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `c0427c8`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
unit	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/1/artifact/out/patch-unit-hadoop-hdds_server-scm.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/1/testReport/
Max. process+thread count	340 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/1/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

arp7

Thanks @nandakumar131 . The code is very beautifully laid out and this patch was a pleasure to review. A few comments.

...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

arp7 · 2019-03-18T23:00:49Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

We are not deleting the 'obsolete' replicas here, right?

Yup, we don't do any replica deletion here.

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

arp7 · 2019-03-18T23:50:57Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

Do we also need to handle the case where state == CLOSED but we have some quasi-closed replicas, so we need to do force-close.

That will be handled by handleInconsistentContainer method

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

hadoop-yetus · 2019-03-19T20:58:26Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	36	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	26	Maven dependency ordering for branch
+1	mvninstall	989	trunk passed
+1	compile	75	trunk passed
+1	checkstyle	30	trunk passed
+1	mvnsite	78	trunk passed
+1	shadedclient	773	branch has no errors when building and testing our client artifacts.
+1	findbugs	111	trunk passed
+1	javadoc	60	trunk passed
		_ Patch Compile Tests _
0	mvndep	12	Maven dependency ordering for patch
+1	mvninstall	71	the patch passed
+1	compile	68	the patch passed
+1	javac	68	the patch passed
+1	checkstyle	24	the patch passed
+1	mvnsite	61	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	xml	2	The patch has no ill-formed XML file.
+1	shadedclient	741	patch has no errors when building and testing our client artifacts.
+1	findbugs	124	the patch passed
+1	javadoc	56	the patch passed
		_ Other Tests _
+1	unit	63	common in the patch passed.
+1	unit	106	server-scm in the patch passed.
+1	asflicense	29	The patch does not generate ASF License warnings.
		3580

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-620/2/artifact/out/Dockerfile
GITHUB PR	#620
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
uname	Linux 6d8b77e65902 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 / `5d8bd0e`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/2/testReport/
Max. process+thread count	412 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/2/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

arp7

+1 LGTM, with one minor comment about handling InterruptedException.

Thanks for working through this so patiently @nandakumar131!

arp7 · 2019-03-20T03:02:16Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

We discussed this offline and I think the agreement was to fix the locking in container report processor.

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

hadoop-yetus · 2019-03-20T19:40:58Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	27	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	24	Maven dependency ordering for branch
+1	mvninstall	976	trunk passed
+1	compile	75	trunk passed
+1	checkstyle	31	trunk passed
+1	mvnsite	80	trunk passed
+1	shadedclient	770	branch has no errors when building and testing our client artifacts.
+1	findbugs	109	trunk passed
+1	javadoc	64	trunk passed
		_ Patch Compile Tests _
0	mvndep	13	Maven dependency ordering for patch
+1	mvninstall	73	the patch passed
+1	compile	70	the patch passed
+1	javac	70	the patch passed
+1	checkstyle	25	the patch passed
+1	mvnsite	62	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	xml	2	The patch has no ill-formed XML file.
+1	shadedclient	737	patch has no errors when building and testing our client artifacts.
-1	findbugs	49	hadoop-hdds/server-scm generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1	javadoc	60	the patch passed
		_ Other Tests _
+1	unit	63	common in the patch passed.
+1	unit	106	server-scm in the patch passed.
+1	asflicense	30	The patch does not generate ASF License warnings.
		3549

Reason	Tests
FindBugs	module:hadoop-hdds/server-scm
	Naked notify in org.apache.hadoop.hdds.scm.container.ReplicationManager.processContainersNow() At ReplicationManager.java:At ReplicationManager.java:[line 176]

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-620/3/artifact/out/Dockerfile
GITHUB PR	#620
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
uname	Linux a7363712cbed 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 / `b3b0e33`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
findbugs	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/3/artifact/out/new-findbugs-hadoop-hdds_server-scm.html
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/3/testReport/
Max. process+thread count	419 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/3/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

arp7

Two minor comments.

arp7 · 2019-03-20T22:56:39Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

Don't make the method synchronized. If so then anyone who invokes processContainersNow will block indefinitely while the whole iteration completes. Instead just wrap the wait(interval) inside a synchronized(this) block.

Also you will have to suppress the findbugs warning about the naked notify call.

arp7 · 2019-03-20T23:36:37Z

...p-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java

Don't call terminate if running is false. In that case, just exit the thread. See RedundancyMonitor#run in HDFS BlockManager.java.

arp7 · 2019-03-21T05:19:29Z

+1 LGTM.

hadoop-yetus · 2019-03-21T12:32:37Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	0	Docker mode activated.
-1	patch	7	#620 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.

Subsystem	Report/Notes
GITHUB PR	#620
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/4/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2019-03-21T12:50:57Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	0	Docker mode activated.
-1	patch	7	#620 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.

Subsystem	Report/Notes
GITHUB PR	#620
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/5/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

nandakumar131 · 2019-03-21T12:58:02Z

Not sure why yetus is failing, the branch is up to date with trunk.

arp7 · 2019-03-21T20:01:45Z

+1 from me. Not sure why Yetus is failing either.

…ers. Contributed by Nanda kumar.

arp7 · 2019-03-22T19:41:40Z

Squashed and rebased on top of Apache trunk to get Jenkins results.

hadoop-yetus · 2019-03-22T20:32:16Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	20	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	30	Maven dependency ordering for branch
+1	mvninstall	1067	trunk passed
+1	compile	90	trunk passed
+1	checkstyle	32	trunk passed
+1	mvnsite	82	trunk passed
+1	shadedclient	802	branch has no errors when building and testing our client artifacts.
+1	findbugs	114	trunk passed
+1	javadoc	57	trunk passed
		_ Patch Compile Tests _
0	mvndep	11	Maven dependency ordering for patch
+1	mvninstall	73	the patch passed
+1	compile	71	the patch passed
+1	javac	71	the patch passed
+1	checkstyle	25	the patch passed
+1	mvnsite	63	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	xml	1	The patch has no ill-formed XML file.
+1	shadedclient	776	patch has no errors when building and testing our client artifacts.
+1	findbugs	127	the patch passed
+1	javadoc	54	the patch passed
		_ Other Tests _
+1	unit	78	common in the patch passed.
+1	unit	89	server-scm in the patch passed.
+1	asflicense	25	The patch does not generate ASF License warnings.
		3720

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-620/6/artifact/out/Dockerfile
GITHUB PR	#620
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
uname	Linux 249fc2c55ea3 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `509b20b`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/6/testReport/
Max. process+thread count	364 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/6/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2019-03-22T20:43:50Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	20	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	10	Maven dependency ordering for branch
+1	mvninstall	1083	trunk passed
+1	compile	73	trunk passed
+1	checkstyle	28	trunk passed
+1	mvnsite	70	trunk passed
+1	shadedclient	773	branch has no errors when building and testing our client artifacts.
+1	findbugs	110	trunk passed
+1	javadoc	56	trunk passed
		_ Patch Compile Tests _
0	mvndep	11	Maven dependency ordering for patch
+1	mvninstall	72	the patch passed
+1	compile	69	the patch passed
+1	javac	69	the patch passed
+1	checkstyle	25	the patch passed
+1	mvnsite	62	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	xml	1	The patch has no ill-formed XML file.
+1	shadedclient	789	patch has no errors when building and testing our client artifacts.
+1	findbugs	123	the patch passed
+1	javadoc	57	the patch passed
		_ Other Tests _
+1	unit	80	common in the patch passed.
+1	unit	94	server-scm in the patch passed.
+1	asflicense	24	The patch does not generate ASF License warnings.
		3679

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-620/7/artifact/out/Dockerfile
GITHUB PR	#620
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
uname	Linux da08a7ec4e84 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `509b20b`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/7/testReport/
Max. process+thread count	336 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common hadoop-hdds/server-scm U: hadoop-hdds
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-620/7/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

… apply configuration overrides Our integration test framework uses configuration overrides (i.e. jobs.*) to override the user system configuration set in the code (e.g. KafkaSystemDescriptor) to test systems. However, the StreamManager we created before calling ExecutionPlanner.plan() does not apply the overrides and causes failure in tests since the system was not correctly set to in-memory systems by configuration overrides. Author: Yi Pan (Data Infrastructure) <[email protected]> Reviewers: Prateek Maheshwari <[email protected]>, Boris S <[email protected]>, Xinyu Liu <[email protected]>, Shanthoosh Venkataraman <[email protected]> Closes apache#620 from nickpan47/SAMZA-1836 and squashes the following commits: a376b888 [Yi Pan (Data Infrastructure)] SAMZA-1836: StreamManager created before ExecutionPlanner should also apply the configuration overrides 35f2c0b7 [Yi Pan (Data Infrastructure)] SAMZA-1836: fixing two unit tests that should use InMemorySystemFactory instead of KafkaSystemDescriptor

arp7 assigned nandakumar131 Mar 18, 2019

arp7 requested a review from nandakumar131 March 18, 2019 20:21

arp7 assigned arp7 and unassigned arp7 Mar 18, 2019

arp7 commented Mar 19, 2019

View reviewed changes

arp7 removed the request for review from nandakumar131 March 19, 2019 00:04

arp7 commented Mar 20, 2019

View reviewed changes

arp7 force-pushed the HDDS-1205 branch from d607b1b to b4c21a1 Compare March 22, 2019 19:29

HDDS-1205. Refactor ReplicationManager to handle QUASI_CLOSED contain…

34a331b

…ers. Contributed by Nanda kumar.

arp7 force-pushed the HDDS-1205 branch from b4c21a1 to 34a331b Compare March 22, 2019 19:41

arp7 merged commit f854a89 into apache:trunk Mar 22, 2019

nandakumar131 mentioned this pull request Mar 29, 2019

HDDS-1207. Refactor Container Report Processing logic and plugin new Replication Manager. #662

Merged

nandakumar131 mentioned this pull request Apr 9, 2019

HDDS-1368. Cleanup old ReplicationManager code from SCM. #711

Closed

nandakumar131 removed their assignment Dec 17, 2019

HDDS-1205. Refactor ReplicationManager to handle QUASI_CLOSED contain… #620

HDDS-1205. Refactor ReplicationManager to handle QUASI_CLOSED contain… #620

Uh oh!

Conversation

arp7 commented Mar 18, 2019

Uh oh!

hadoop-yetus commented Mar 18, 2019

Uh oh!

arp7 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hadoop-yetus commented Mar 19, 2019

Uh oh!

arp7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hadoop-yetus commented Mar 20, 2019

Uh oh!

arp7 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!

Choose a reason for hiding this comment

Uh oh!

arp7 commented Mar 21, 2019

Uh oh!

hadoop-yetus commented Mar 21, 2019

Uh oh!

hadoop-yetus commented Mar 21, 2019

Uh oh!

nandakumar131 commented Mar 21, 2019

Uh oh!

arp7 commented Mar 21, 2019

Uh oh!

arp7 commented Mar 22, 2019

Uh oh!

hadoop-yetus commented Mar 22, 2019

Uh oh!

hadoop-yetus commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants