Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-1491. Ozone KeyInputStream seek() should not read the chunk file. #795

Merged
merged 8 commits into from May 14, 2019

Conversation

hanishakoneru
Copy link
Contributor

No description provided.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@bshashikant
Copy link
Contributor

Thanks @hanishakoneru for working on this. The patch overall looks good to me. Some minor comments:

  1. BlockInputStream.java : 81 : update javadoc
  2. BlockInputStream.java : 392 : correct the comment
  3. It would be good to add some metrics similar to xceiverClientRatis in XceiverClientGrpc and add some tests to verify that no reads are happening as a part of seek using the metrics.

@hanishakoneru
Copy link
Contributor Author

Thank you @bshashikant for the review. I have addressed your review comments.
Also, there was a mistake in bufferPosition. It should be wrt the buffers corresponding to a chunk. Fixed that too.

@hadoop-yetus

This comment has been minimized.

@bshashikant
Copy link
Contributor

Thanks @hanishakoneru for updating the patch. Test failures are related to the patch. Can you please check?

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@bshashikant
Copy link
Contributor

bshashikant commented May 13, 2019

Thanks @hanishakoneru for updating the patch. The patch looks good to me. I am +1 on this.
Please address the checkStyle issues while committing.

We can also add tests with read failures in TestKeyInputStream, but these can be added as a part of separate jira.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 856 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 4 new or modified test files.
_ trunk Compile Tests _
0 mvndep 60 Maven dependency ordering for branch
+1 mvninstall 376 trunk passed
+1 compile 189 trunk passed
+1 checkstyle 45 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 759 branch has no errors when building and testing our client artifacts.
+1 javadoc 124 trunk passed
0 spotbugs 238 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 414 trunk passed
_ Patch Compile Tests _
0 mvndep 26 Maven dependency ordering for patch
+1 mvninstall 399 the patch passed
+1 compile 211 the patch passed
+1 javac 211 the patch passed
+1 checkstyle 52 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 668 patch has no errors when building and testing our client artifacts.
+1 javadoc 124 the patch passed
+1 findbugs 435 the patch passed
_ Other Tests _
-1 unit 163 hadoop-hdds in the patch failed.
-1 unit 1344 hadoop-ozone in the patch failed.
+1 asflicense 37 The patch does not generate ASF License warnings.
6424
Reason Tests
Failed junit tests hadoop.hdds.scm.pipeline.TestRatisPipelineProvider
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
hadoop.ozone.client.rpc.TestWatchForCommit
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-795/8/artifact/out/Dockerfile
GITHUB PR #795
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 8529a64e9393 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 / 389e640
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-795/8/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-795/8/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-795/8/testReport/
Max. process+thread count 5178 (vs. ulimit of 5500)
modules C: hadoop-hdds/client hadoop-ozone/client hadoop-ozone/integration-test hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-795/8/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
Copy link
Contributor Author

/retest

@hanishakoneru
Copy link
Contributor Author

Thank you @bshashikant .
I fixed the checkstyle issue and run checkstyle inspection locally. Looks like CI is not working currently. I am going to merge this PR. There is no change in the last update except for checkstyle fixes. Verified locally that everything is working well.

@hanishakoneru hanishakoneru merged commit 02c9efc into apache:trunk May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants