HDDS-1258 - Fix error propagation for SCM protocol #1001

sodonnel · 2019-06-21T10:59:02Z

Following on from HDDS-1674, which changed the SCMBlockLocationProtocol to use a single wrapper message for all child messages, this change ensures that any known SCMException's are caught and the correct code and message written to the wrapper.

Note that the client does not see the benifit of the new error handling, as the generally the client calls CLIENT -> OM -> SCM, and even with this change the OM cannot translate the SCM response codes into a more meaningful error message for the client.

sodonnel · 2019-06-21T10:59:27Z

/label ozone

hadoop-yetus · 2019-06-21T12:51:09Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	43	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 1 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	30	Maven dependency ordering for branch
+1	mvninstall	505	trunk passed
+1	compile	252	trunk passed
+1	checkstyle	71	trunk passed
+1	mvnsite	0	trunk passed
+1	shadedclient	903	branch has no errors when building and testing our client artifacts.
+1	javadoc	151	trunk passed
0	spotbugs	331	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	526	trunk passed
		_ Patch Compile Tests _
0	mvndep	20	Maven dependency ordering for patch
+1	mvninstall	454	the patch passed
+1	compile	263	the patch passed
+1	cc	263	the patch passed
+1	javac	263	the patch passed
+1	checkstyle	76	the patch passed
+1	mvnsite	0	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	742	patch has no errors when building and testing our client artifacts.
+1	javadoc	155	the patch passed
+1	findbugs	545	the patch passed
		_ Other Tests _
+1	unit	275	hadoop-hdds in the patch passed.
-1	unit	1453	hadoop-ozone in the patch failed.
+1	asflicense	47	The patch does not generate ASF License warnings.
		6660

Reason	Tests
Failed junit tests	hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
	hadoop.ozone.client.rpc.TestWatchForCommit
	hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
	hadoop.ozone.client.rpc.TestCommitWatcher
	hadoop.ozone.client.rpc.TestOzoneRpcClient

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/1/artifact/out/Dockerfile
GITHUB PR	#1001
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname	Linux ef06faf1f90f 4.4.0-141-generic #167~14.04.1-Ubuntu SMP Mon Dec 10 13:20:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `83dcb9d`
Default Java	1.8.0_212
unit	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/1/artifact/out/patch-unit-hadoop-ozone.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/1/testReport/
Max. process+thread count	5055 (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-1001/1/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.

jiwq

At first we need the unify license style.

sodonnel · 2019-06-21T13:53:36Z

@jiwq I am not sure what you mean by "the unify license style"? Can you point to the file with the issue and what I should do to fix it please?

I ran all the failing tests locally and they all passed, so I think they are unrelated to this change.

jiwq

I will review again after you fixed these nits. Thanks!

jiwq · 2019-06-21T16:25:03Z

.../common/src/test/java/org/apache/hadoop/hdds/scm/exceptions/TestSCMExceptionResultCodes.java

Suggested change

* <p>

*

jiwq · 2019-06-21T16:25:16Z

.../common/src/test/java/org/apache/hadoop/hdds/scm/exceptions/TestSCMExceptionResultCodes.java

Suggested change

* <p>

*

jiwq · 2019-06-21T16:25:26Z

.../common/src/test/java/org/apache/hadoop/hdds/scm/exceptions/TestSCMExceptionResultCodes.java

Suggested change

* http://www.apache.org/licenses/LICENSE-2.0

* http://www.apache.org/licenses/LICENSE-2.0

jiwq · 2019-06-21T16:25:44Z

.../common/src/test/java/org/apache/hadoop/hdds/scm/exceptions/TestSCMExceptionResultCodes.java

Suggested change

}

}

sodonnel · 2019-06-21T17:14:56Z

I have push a new version with the formatting issues hopefully addressed.

hadoop-yetus · 2019-06-21T19:22:10Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	45	Docker mode activated.
		_ Prechecks _
+1	dupname	0	No case conflicting files found.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 1 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	13	Maven dependency ordering for branch
+1	mvninstall	542	trunk passed
+1	compile	294	trunk passed
+1	checkstyle	84	trunk passed
+1	mvnsite	0	trunk passed
+1	shadedclient	903	branch has no errors when building and testing our client artifacts.
+1	javadoc	158	trunk passed
0	spotbugs	356	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	560	trunk passed
		_ Patch Compile Tests _
0	mvndep	21	Maven dependency ordering for patch
+1	mvninstall	473	the patch passed
+1	compile	302	the patch passed
+1	cc	302	the patch passed
+1	javac	302	the patch passed
+1	checkstyle	73	the patch passed
+1	mvnsite	0	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	821	patch has no errors when building and testing our client artifacts.
+1	javadoc	187	the patch passed
+1	findbugs	601	the patch passed
		_ Other Tests _
+1	unit	313	hadoop-hdds in the patch passed.
-1	unit	2057	hadoop-ozone in the patch failed.
+1	asflicense	51	The patch does not generate ASF License warnings.
		7654

Reason	Tests
Failed junit tests	hadoop.ozone.client.rpc.TestBCSID
	hadoop.ozone.container.common.statemachine.commandhandler.TestBlockDeletion
	hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerByPipeline
	hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/2/artifact/out/Dockerfile
GITHUB PR	#1001
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname	Linux 4569f10c283e 4.4.0-141-generic #167~14.04.1-Ubuntu SMP Mon Dec 10 13:20:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `cba13c7`
Default Java	1.8.0_212
unit	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/2/artifact/out/patch-unit-hadoop-ozone.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/2/testReport/
Max. process+thread count	4501 (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-1001/2/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.

…atus in the wrapper message and throw SCMException for expected error conditions.

hadoop-yetus · 2019-06-25T12:15:42Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	32	Docker mode activated.
		_ Prechecks _
+1	dupname	0	No case conflicting files found.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 1 new or modified test files.
		_ trunk Compile Tests _
+1	mvninstall	523	trunk passed
+1	compile	269	trunk passed
+1	checkstyle	68	trunk passed
+1	mvnsite	0	trunk passed
+1	shadedclient	841	branch has no errors when building and testing our client artifacts.
+1	javadoc	149	trunk passed
0	spotbugs	335	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	542	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	485	the patch passed
+1	compile	266	the patch passed
+1	cc	266	the patch passed
+1	javac	266	the patch passed
+1	checkstyle	68	the patch passed
+1	mvnsite	0	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	658	patch has no errors when building and testing our client artifacts.
+1	javadoc	156	the patch passed
+1	findbugs	549	the patch passed
		_ Other Tests _
-1	unit	155	hadoop-hdds in the patch failed.
-1	unit	721	hadoop-ozone in the patch failed.
+1	asflicense	39	The patch does not generate ASF License warnings.
		5672

Reason	Tests
Failed junit tests	hadoop.ozone.container.common.statemachine.commandhandler.TestCloseContainerCommandHandler
	hadoop.ozone.web.client.TestKeysRatis
	hadoop.ozone.web.client.TestKeys
	hadoop.ozone.client.rpc.TestReadRetries
	hadoop.ozone.om.TestScmSafeMode

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/3/artifact/out/Dockerfile
GITHUB PR	#1001
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname	Linux 2400e99c3cf4 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 / `48d7f00`
Default Java	1.8.0_212
unit	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/3/artifact/out/patch-unit-hadoop-hdds.txt
unit	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/3/artifact/out/patch-unit-hadoop-ozone.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/3/testReport/
Max. process+thread count	2350 (vs. ulimit of 5500)
modules	C: hadoop-hdds/common U: hadoop-hdds/common
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-1001/3/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.

sodonnel · 2019-06-26T10:08:47Z

/retest

elek · 2019-07-01T18:13:40Z

Wow, full green build check with no intermittent failures. How did you do that? ;-)

elek

+1 LGTM

Thank you the nice work @sodonnel

Now we have the opportunity to propagate the errors and do smart error conversion between scm errors -> om errors.

Closes apache#1001

elek added the ozone label Jun 21, 2019

jiwq reviewed Jun 21, 2019

View reviewed changes

jiwq suggested changes Jun 21, 2019

View reviewed changes

S O'Donnell added 2 commits June 25, 2019 11:39

Refactor Type Base SCMBlockLocationProtocol to pass the errors and st…

1b4b5d5

…atus in the wrapper message and throw SCMException for expected error conditions.

Fix formatting issues raised in review

92f33fc

sodonnel force-pushed the HDDS-1258 branch from 843e319 to 92f33fc Compare June 25, 2019 10:40

elek approved these changes Jul 1, 2019

View reviewed changes

elek closed this in f8d62a9 Jul 1, 2019

bshashikant pushed a commit to bshashikant/hadoop that referenced this pull request Jul 10, 2019

HDDS-1258. Fix error propagation for SCM protocol

a25cfc1

Closes apache#1001

amahussein pushed a commit to amahussein/hadoop that referenced this pull request Oct 29, 2019

HDDS-1258. Fix error propagation for SCM protocol

281e119

Closes apache#1001

	* http://www.apache.org/licenses/LICENSE-2.0
	* http://www.apache.org/licenses/LICENSE-2.0

HDDS-1258 - Fix error propagation for SCM protocol #1001

HDDS-1258 - Fix error propagation for SCM protocol #1001

Uh oh!

Conversation

sodonnel commented Jun 21, 2019

Uh oh!

sodonnel commented Jun 21, 2019

Uh oh!

hadoop-yetus commented Jun 21, 2019

Uh oh!

jiwq left a comment

Choose a reason for hiding this comment

Uh oh!

sodonnel commented Jun 21, 2019

Uh oh!

jiwq left a comment

Choose a reason for hiding this comment

Uh oh!

jiwq Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

jiwq Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

jiwq Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

jiwq Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

sodonnel commented Jun 21, 2019

Uh oh!

hadoop-yetus commented Jun 21, 2019

Uh oh!

hadoop-yetus commented Jun 25, 2019

Uh oh!

sodonnel commented Jun 26, 2019

Uh oh!

elek commented Jul 1, 2019

Uh oh!

elek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants