HDDS-1496. Support partial chunk reads and checksum verification #804

hanishakoneru · 2019-05-08T22:32:20Z

Partial chunk reads and checksum verifications

hanishakoneru · 2019-05-09T17:43:51Z

This patch requires more changes (after HDDS-1491 which fixes seek operation).

In BlockInputStream#seek(), it is not sufficient to just check if the required chunkIndex matches with current buffers chunk index and that the buffer has data remaining. Since the buffer might have only a partial chunk, it is possible that it does not cover the position seeked.
In BlockInputStream#readChunkFromContainer(), we should not blindly increment the chunkIndex. The last read might have read only a part of the chunk and the next read may be required to read from the same chunk again.

hanishakoneru · 2019-05-14T01:16:04Z

@bshashikant , @arp7
This PR consists of the patch for Partial Chunk Reads on top of HDDS-1491. The second commit is for the current PR. I posted this way so that we can get started with the review for this PR without getting blocked by HDDS-1491 (which has only checkstyle sign off remaining).

hanishakoneru · 2019-05-14T04:13:22Z

Merger HDDS-1491 and updated this PR to include only changes for this Jira.

hanishakoneru · 2019-05-23T01:13:28Z

Introduced ChunkInputStream and separated the chunk reads from BlockInputStream.
This is an initial patch. I am working on unit tests for this.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyInputStream.java

bharatviswa504 · 2019-05-23T18:17:36Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

NIT: Existing only, but is java doc needs to be modified? As it is saying used by Rest Service and ScmClient.
This is used by KeyInputStream, and RpcClient also uses this, for reading data from container chunks we talk to datanode using storageContainer protocol client.

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyInputStream.java

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

bharatviswa504 · 2019-05-23T19:10:19Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

Why here returning false, read Javadoc not completely clear of this?

I think returning false here as we do not implement it.

bharatviswa504 · 2019-05-23T19:16:13Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java

Same as above java doc description needs modification.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java

bharatviswa504

I have some minor comments. Still reviewing ChunkInputStream class.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestChunkInputStream.java

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockInputStream.java

bharatviswa504

Hi @hanishakoneru
Thanks for the improvement.
I have a few minor comments.

Redesigned Ozone InputStreams Fixes review comments and CI fixes Findbug and checkstyle fixes

hanishakoneru · 2019-06-05T01:34:01Z

/retest

bharatviswa504 · 2019-06-05T21:48:23Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java

+    if (initialized) {
+      return;
+    }
+    Preconditions.checkArgument(chunkOffsets == null);


Do we need this Precondition check?

Just extra precaution so that if new methods are implemented then the stream is not initialized twice. It is not necessary for now though. Removed it.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java

bharatviswa504 · 2019-06-05T22:28:23Z

Hi @hanishakoneru
Thanks for the update. I see some of the old comments are not addressed.
I have a few minor Nits.

bharatviswa504

Thanks for the update @hanishakoneru
+1 LGTM, pending CI.

hanishakoneru · 2019-06-06T22:01:41Z

/retest

hadoop-yetus · 2019-06-06T22:20:15Z

💔 -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 3 new or modified test files.
		_ trunk Compile Tests _
0	mvndep	36	Maven dependency ordering for branch
+1	mvninstall	533	trunk passed
+1	compile	289	trunk passed
+1	checkstyle	88	trunk passed
+1	mvnsite	0	trunk passed
+1	shadedclient	952	branch has no errors when building and testing our client artifacts.
+1	javadoc	182	trunk passed
0	spotbugs	357	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	571	trunk passed
		_ Patch Compile Tests _
0	mvndep	31	Maven dependency ordering for patch
+1	mvninstall	491	the patch passed
-1	compile	79	hadoop-ozone in the patch failed.
-1	javac	79	hadoop-ozone in the patch failed.
-0	checkstyle	45	hadoop-hdds: The patch generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
+1	mvnsite	0	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	730	patch has no errors when building and testing our client artifacts.
+1	javadoc	186	the patch passed
-1	findbugs	87	hadoop-ozone in the patch failed.
		_ Other Tests _
-1	unit	168	hadoop-hdds in the patch failed.
-1	unit	58	hadoop-ozone in the patch failed.
+1	asflicense	37	The patch does not generate ASF License warnings.
		5136

Reason	Tests
Failed junit tests	hadoop.ozone.container.common.impl.TestHddsDispatcher

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

hanishakoneru · 2019-06-07T02:43:09Z

Thank you @bharatviswa504 for the reviews.
There was minor checkstyle issue (unused import) which i fixed. The unit test failure is not related to this patch and passes locally. I will merge this PR.

hadoop-yetus · 2019-06-07T02:45:10Z

💔 -1 overall

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

Subsystem	Report/Notes
Docker	Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-804/14/artifact/out/Dockerfile
GITHUB PR	#804
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-804/14/console
versions	git=2.7.4
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

hanishakoneru added the ozone label May 8, 2019

hanishakoneru requested review from arp7 and bshashikant May 8, 2019 22:32

This comment has been minimized.

Sign in to view

hanishakoneru force-pushed the HDDS-1496 branch from f6c11c9 to fdf6d95 Compare May 14, 2019 01:12

hanishakoneru force-pushed the HDDS-1496 branch from fdf6d95 to 165c5cd Compare May 14, 2019 04:12

This comment has been minimized.

Sign in to view

arp7 assigned hanishakoneru May 14, 2019

This comment has been minimized.

Sign in to view

hanishakoneru force-pushed the HDDS-1496 branch from c5ab703 to b20f823 Compare May 23, 2019 01:11

hadoop-yetus reviewed May 23, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 23, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 23, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 23, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 23, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bharatviswa504 reviewed May 23, 2019

View reviewed changes

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

hadoop-yetus reviewed May 24, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestChunkInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestChunkInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestChunkInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockInputStream.java Outdated Show resolved Hide resolved

bharatviswa504 reviewed Jun 3, 2019

View reviewed changes

hanishakoneru force-pushed the HDDS-1496 branch from 831ec4c to 173cf1b Compare June 4, 2019 00:27

This comment has been minimized.

Sign in to view

hanishakoneru added 2 commits June 4, 2019 12:22

Partial Chunk reads

fc5eb36

Redesigned Ozone InputStreams Fixes review comments and CI fixes Findbug and checkstyle fixes

rebase

6af6439

hanishakoneru force-pushed the HDDS-1496 branch from 173cf1b to 6af6439 Compare June 4, 2019 20:21

This comment has been minimized.

Sign in to view

FindBug and Checkstyle fixes

b31231d

adding inheritDoc

ae9297f

This comment has been minimized.

Sign in to view

checkstyle

fea54dd

bharatviswa504 reviewed Jun 5, 2019

View reviewed changes

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ChunkInputStream.java Outdated Show resolved Hide resolved

Pending review comments

b2a1970

bharatviswa504 approved these changes Jun 6, 2019

View reviewed changes

Unused imports

e4f701e

hanishakoneru merged commit a91d24f into apache:trunk Jun 7, 2019

HDDS-1496. Support partial chunk reads and checksum verification #804

HDDS-1496. Support partial chunk reads and checksum verification #804

Uh oh!

Conversation

hanishakoneru commented May 8, 2019

Uh oh!

This comment has been minimized.

hanishakoneru commented May 9, 2019

Uh oh!

hanishakoneru commented May 14, 2019

Uh oh!

hanishakoneru commented May 14, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hanishakoneru commented May 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

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!

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!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

hanishakoneru commented Jun 5, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bharatviswa504 commented Jun 5, 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!

hanishakoneru commented Jun 6, 2019

bharatviswa504 left a comment •

edited

Loading