HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHandler #707

elek · 2019-04-08T16:28:38Z

When I analyzed the usages of the available RPC protocols in Ozone I found that the ScmBlockLocationProtocol is not used in ObjectStore at all.

I would propose to remove it...

See: https://issues.apache.org/jira/browse/HDDS-1402

hadoop-yetus · 2019-04-08T18:14:39Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	26	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
-1	test4tests	0	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ trunk Compile Tests _
+1	mvninstall	1005	trunk passed
+1	compile	48	trunk passed
+1	checkstyle	18	trunk passed
+1	mvnsite	26	trunk passed
+1	shadedclient	667	branch has no errors when building and testing our client artifacts.
+1	findbugs	38	trunk passed
+1	javadoc	26	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	41	the patch passed
+1	compile	18	the patch passed
+1	javac	18	the patch passed
+1	checkstyle	11	the patch passed
+1	mvnsite	22	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	710	patch has no errors when building and testing our client artifacts.
-1	findbugs	44	hadoop-ozone/objectstore-service generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1	javadoc	19	the patch passed
		_ Other Tests _
+1	unit	23	objectstore-service in the patch passed.
+1	asflicense	30	The patch does not generate ASF License warnings.
		2876

Reason	Tests
FindBugs	module:hadoop-ozone/objectstore-service
	Dead store to scmBlockAddress in new org.apache.hadoop.hdfs.server.datanode.ObjectStoreHandler(Configuration) At ObjectStoreHandler.java:new org.apache.hadoop.hdfs.server.datanode.ObjectStoreHandler(Configuration) At ObjectStoreHandler.java:[line 106]

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-707/1/artifact/out/Dockerfile
GITHUB PR	#707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux 0d186edb2f23 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 / `69e3745`
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-707/1/artifact/out/new-findbugs-hadoop-ozone_objectstore-service.html
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/1/testReport/
Max. process+thread count	446 (vs. ulimit of 5500)
modules	C: hadoop-ozone/objectstore-service U: hadoop-ozone/objectstore-service
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/1/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2019-04-10T15:01:09Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	74	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
-1	test4tests	0	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ trunk Compile Tests _
+1	mvninstall	1431	trunk passed
+1	compile	52	trunk passed
+1	checkstyle	16	trunk passed
+1	mvnsite	26	trunk passed
+1	shadedclient	868	branch has no errors when building and testing our client artifacts.
+1	findbugs	49	trunk passed
+1	javadoc	26	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	57	the patch passed
+1	compile	24	the patch passed
+1	javac	24	the patch passed
-0	checkstyle	13	hadoop-ozone/objectstore-service: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1	mvnsite	27	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	892	patch has no errors when building and testing our client artifacts.
+1	findbugs	41	the patch passed
+1	javadoc	19	the patch passed
		_ Other Tests _
+1	unit	26	objectstore-service in the patch passed.
+1	asflicense	28	The patch does not generate ASF License warnings.
		3776

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-707/2/artifact/out/Dockerfile
GITHUB PR	#707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux a8d469745182 4.4.0-144-generic #170~14.04.1-Ubuntu SMP Mon Mar 18 15:02:05 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `df01469`
maven	version: Apache Maven 3.3.9
Default Java	1.8.0_191
findbugs	v3.1.0-RC1
checkstyle	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/2/artifact/out/diff-checkstyle-hadoop-ozone_objectstore-service.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/2/testReport/
Max. process+thread count	348 (vs. ulimit of 5500)
modules	C: hadoop-ozone/objectstore-service U: hadoop-ozone/objectstore-service
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/2/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

nandakumar131 · 2019-04-12T09:44:32Z

@elek The changes look good. We have a checkstyle issue which is related. Test failure is not related to this change and it is tracked in HDDS-1413.

…dler. Contributed by Elek, Marton.

elek · 2019-04-12T11:38:14Z

Thanks the review @nandakumar131 Fixed the checkstyle issue (and rebased)

hadoop-yetus · 2019-04-12T13:19:37Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	22	Docker mode activated.
		_ Prechecks _
+1	@author	0	The patch does not contain any @author tags.
-1	test4tests	0	The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ trunk Compile Tests _
+1	mvninstall	1181	trunk passed
+1	compile	50	trunk passed
+1	checkstyle	19	trunk passed
+1	mvnsite	28	trunk passed
+1	shadedclient	869	branch has no errors when building and testing our client artifacts.
+1	findbugs	43	trunk passed
+1	javadoc	29	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	51	the patch passed
+1	compile	22	the patch passed
+1	javac	22	the patch passed
+1	checkstyle	16	the patch passed
+1	mvnsite	26	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	1011	patch has no errors when building and testing our client artifacts.
+1	findbugs	49	the patch passed
+1	javadoc	24	the patch passed
		_ Other Tests _
+1	unit	32	objectstore-service in the patch passed.
+1	asflicense	65	The patch does not generate ASF License warnings.
		3654

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-707/3/artifact/out/Dockerfile
GITHUB PR	#707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux b4a77c63e2e9 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 / `abace70`
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-707/3/testReport/
Max. process+thread count	295 (vs. ulimit of 5500)
modules	C: hadoop-ozone/objectstore-service U: hadoop-ozone/objectstore-service
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-707/3/console
Powered by	Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

bharatviswa504

+1 LGTM.
Thank You @elek for the fix and @nandakumar131 for the review.

We have scattered logic to determine the store paths between SamzaContainer and StorageManager. Ideally, it would nice to consolidate them into StorageManager. For now, using different store directories for non-logged and logged stores so that regressions in determining store paths can be caught. Author: bharathkk <codin.martial@gmail.com> Reviewers: Prateek Maheshwari <pmaheshwari@apache.org> Closes apache#707 from bharathkk/fix-table-test

elek added the ozone label Apr 8, 2019

elek force-pushed the HDDS-1402 branch from d010d1e to 1215fae Compare April 10, 2019 13:07

elek added 3 commits April 12, 2019 13:37

HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHan…

97a2405

…dler. Contributed by Elek, Marton.

findbugs fix

6a173aa

remove unused import (thx checkstyle)

f15cdc0

elek force-pushed the HDDS-1402 branch from ed9d9f0 to f15cdc0 Compare April 12, 2019 11:37

bharatviswa504 approved these changes Apr 17, 2019

View reviewed changes

bharatviswa504 merged commit 04c0437 into apache:trunk Apr 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHandler #707

HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHandler #707

Uh oh!

elek commented Apr 8, 2019

Uh oh!

hadoop-yetus commented Apr 8, 2019

Uh oh!

hadoop-yetus commented Apr 10, 2019

Uh oh!

nandakumar131 commented Apr 12, 2019

Uh oh!

elek commented Apr 12, 2019

Uh oh!

hadoop-yetus commented Apr 12, 2019

Uh oh!

bharatviswa504 left a comment •

edited

Loading

Uh oh!

Uh oh!

HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHandler #707

HDDS-1402. Remove unused ScmBlockLocationProtocol from ObjectStoreHandler #707

Uh oh!

Conversation

elek commented Apr 8, 2019

Uh oh!

hadoop-yetus commented Apr 8, 2019

Uh oh!

hadoop-yetus commented Apr 10, 2019

Uh oh!

nandakumar131 commented Apr 12, 2019

Uh oh!

elek commented Apr 12, 2019

Uh oh!

hadoop-yetus commented Apr 12, 2019

Uh oh!

bharatviswa504 left a comment • 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!

Uh oh!

bharatviswa504 left a comment •

edited

Loading