Skip to content

Conversation

bharatviswa504
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.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline at end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I still don't see a newline. It's a minor point. See https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file

This is an old habit from using a C++ compiler that would throw errors on files not ending in newlines. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the new line, in my IDE is see it, not sure GitHub UI is not showing that.

Screen Shot 2019-07-17 at 4 42 01 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-07-17 at 4 43 02 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline please. I missed this in general, it's good to have files end in newlines.

Perhaps not true these days but historically some tools used to choke on text files that don't end in newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@arp7 arp7 Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If status is OK then omBucketCreateResponse must be non-null right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But this check is added, because we have fields in constructor annotated as nullable, so find bugs will catch us, to avoid that added this check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of that you can add a Preconditions.checkState.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks pretty good overall. A few minor comments/questions.

@bharatviswa504
Copy link
Contributor Author

Thank You @arp7 for the review.
I have addressed the review comments.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 59 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 6 new or modified test files.
_ trunk Compile Tests _
0 mvndep 71 Maven dependency ordering for branch
+1 mvninstall 550 trunk passed
+1 compile 261 trunk passed
+1 checkstyle 70 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 848 branch has no errors when building and testing our client artifacts.
+1 javadoc 156 trunk passed
0 spotbugs 335 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 529 trunk passed
_ Patch Compile Tests _
0 mvndep 32 Maven dependency ordering for patch
+1 mvninstall 448 the patch passed
+1 compile 248 the patch passed
+1 cc 248 the patch passed
+1 javac 248 the patch passed
+1 checkstyle 66 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 618 patch has no errors when building and testing our client artifacts.
-1 javadoc 84 hadoop-ozone generated 2 new + 13 unchanged - 0 fixed = 15 total (was 13)
-1 findbugs 100 hadoop-ozone in the patch failed.
_ Other Tests _
-1 unit 331 hadoop-hdds in the patch failed.
-1 unit 101 hadoop-ozone in the patch failed.
+1 asflicense 33 The patch does not generate ASF License warnings.
5019
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
Subsystem Report/Notes
Docker Client=18.09.8 Server=18.09.8 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/artifact/out/Dockerfile
GITHUB PR #1088
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 3310cd41fd9c 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 / 303a7f8
Default Java 1.8.0_212
javadoc https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/artifact/out/diff-javadoc-javadoc-hadoop-ozone.txt
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/artifact/out/patch-findbugs-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/testReport/
Max. process+thread count 487 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/6/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.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 37 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 6 new or modified test files.
_ trunk Compile Tests _
0 mvndep 64 Maven dependency ordering for branch
+1 mvninstall 511 trunk passed
+1 compile 265 trunk passed
+1 checkstyle 76 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 866 branch has no errors when building and testing our client artifacts.
+1 javadoc 156 trunk passed
0 spotbugs 326 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 523 trunk passed
_ Patch Compile Tests _
0 mvndep 32 Maven dependency ordering for patch
+1 mvninstall 442 the patch passed
+1 compile 253 the patch passed
+1 cc 253 the patch passed
+1 javac 253 the patch passed
+1 checkstyle 75 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 648 patch has no errors when building and testing our client artifacts.
+1 javadoc 152 the patch passed
+1 findbugs 531 the patch passed
_ Other Tests _
-1 unit 272 hadoop-hdds in the patch failed.
-1 unit 1397 hadoop-ozone in the patch failed.
+1 asflicense 37 The patch does not generate ASF License warnings.
6487
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
hadoop.ozone.om.TestOzoneManager
hadoop.ozone.client.rpc.TestWatchForCommit
hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.container.ozoneimpl.TestSecureOzoneContainer
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
Subsystem Report/Notes
Docker Client=18.09.8 Server=18.09.8 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/7/artifact/out/Dockerfile
GITHUB PR #1088
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 081a3ea80694 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 / 303a7f8
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/7/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/7/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/7/testReport/
Max. process+thread count 5095 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/7/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.

Copy link
Contributor

@arp7 arp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments and one question. No blockers though, +1 to commit it.

@bharatviswa504
Copy link
Contributor Author

bharatviswa504 commented Jul 17, 2019

Thank You @arp7 for the review.
I have addressed the review comments. And regrading newline on Github UI it is not showing up, when you see in IDE it is showing up. Let me know if you are not still seeing it even in IDE.

@bharatviswa504
Copy link
Contributor Author

/retest

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 39 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 6 new or modified test files.
_ trunk Compile Tests _
0 mvndep 25 Maven dependency ordering for branch
+1 mvninstall 510 trunk passed
+1 compile 274 trunk passed
+1 checkstyle 80 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 871 branch has no errors when building and testing our client artifacts.
+1 javadoc 163 trunk passed
0 spotbugs 318 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 509 trunk passed
_ Patch Compile Tests _
0 mvndep 74 Maven dependency ordering for patch
+1 mvninstall 451 the patch passed
+1 compile 276 the patch passed
+1 cc 276 the patch passed
+1 javac 276 the patch passed
+1 checkstyle 86 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 667 patch has no errors when building and testing our client artifacts.
+1 javadoc 166 the patch passed
+1 findbugs 528 the patch passed
_ Other Tests _
-1 unit 295 hadoop-hdds in the patch failed.
-1 unit 1718 hadoop-ozone in the patch failed.
+1 asflicense 46 The patch does not generate ASF License warnings.
6969
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
hadoop.ozone.container.server.TestSecureContainerServer
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient
hadoop.ozone.client.rpc.TestOzoneAtRestEncryption
hadoop.ozone.container.ozoneimpl.TestSecureOzoneContainer
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
Subsystem Report/Notes
Docker Client=18.09.8 Server=18.09.8 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/8/artifact/out/Dockerfile
GITHUB PR #1088
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux ef49d891ac3b 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 / 5e6cc6f
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/8/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/8/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/8/testReport/
Max. process+thread count 5388 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/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.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 58 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 6 new or modified test files.
_ trunk Compile Tests _
0 mvndep 23 Maven dependency ordering for branch
+1 mvninstall 529 trunk passed
+1 compile 337 trunk passed
+1 checkstyle 61 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 785 branch has no errors when building and testing our client artifacts.
+1 javadoc 149 trunk passed
0 spotbugs 303 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 498 trunk passed
_ Patch Compile Tests _
0 mvndep 28 Maven dependency ordering for patch
+1 mvninstall 414 the patch passed
+1 compile 244 the patch passed
+1 cc 244 the patch passed
+1 javac 244 the patch passed
+1 checkstyle 63 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 608 patch has no errors when building and testing our client artifacts.
+1 javadoc 145 the patch passed
+1 findbugs 527 the patch passed
_ Other Tests _
-1 unit 342 hadoop-hdds in the patch failed.
-1 unit 227 hadoop-ozone in the patch failed.
+1 asflicense 31 The patch does not generate ASF License warnings.
5228
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
hadoop.ozone.om.ratis.TestOzoneManagerDoubleBufferWithOMResponse
Subsystem Report/Notes
Docker Client=18.09.8 Server=18.09.8 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/9/artifact/out/Dockerfile
GITHUB PR #1088
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux fcfc44856afe 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 / 73e6ffc
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/9/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/9/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/9/testReport/
Max. process+thread count 486 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/9/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.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 86 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 6 new or modified test files.
_ trunk Compile Tests _
0 mvndep 83 Maven dependency ordering for branch
+1 mvninstall 517 trunk passed
+1 compile 269 trunk passed
+1 checkstyle 71 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 901 branch has no errors when building and testing our client artifacts.
+1 javadoc 163 trunk passed
0 spotbugs 352 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 564 trunk passed
_ Patch Compile Tests _
0 mvndep 33 Maven dependency ordering for patch
+1 mvninstall 428 the patch passed
+1 compile 278 the patch passed
+1 cc 278 the patch passed
+1 javac 278 the patch passed
+1 checkstyle 152 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 740 patch has no errors when building and testing our client artifacts.
+1 javadoc 155 the patch passed
+1 findbugs 535 the patch passed
_ Other Tests _
-1 unit 341 hadoop-hdds in the patch failed.
-1 unit 2198 hadoop-ozone in the patch failed.
+1 asflicense 42 The patch does not generate ASF License warnings.
7736
Reason Tests
Failed junit tests hadoop.hdds.scm.container.placement.algorithms.TestContainerPlacementFactory
hadoop.ozone.client.rpc.TestCloseContainerHandlingByClient
hadoop.ozone.client.rpc.TestKeyInputStream
hadoop.ozone.client.rpc.TestOzoneRpcClient
hadoop.ozone.client.rpc.TestOzoneRpcClientWithRatis
hadoop.ozone.client.rpc.TestOzoneClientRetriesOnException
hadoop.ozone.client.rpc.TestSecureOzoneRpcClient
hadoop.ozone.container.ozoneimpl.TestSecureOzoneContainer
hadoop.hdds.scm.pipeline.TestRatisPipelineCreateAndDestory
hadoop.ozone.container.server.TestSecureContainerServer
Subsystem Report/Notes
Docker Client=18.09.8 Server=18.09.8 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/10/artifact/out/Dockerfile
GITHUB PR #1088
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 386c9253f42b 4.15.0-48-generic #51-Ubuntu SMP Wed Apr 3 08:28:49 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 73e6ffc
Default Java 1.8.0_212
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/10/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/10/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/10/testReport/
Max. process+thread count 5403 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/common hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1088/10/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.

@bharatviswa504
Copy link
Contributor Author

Test failure is not related to this patch.
Thank You @arp7 for the review.
I will commit this to the trunk.

@bharatviswa504 bharatviswa504 merged commit 3dc256e into apache:trunk Jul 18, 2019
shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
Changing boolean gauges to integer since it is a more widely supported reporter type.

prateekm Please take a look

Author: Daniel Chen <dchen1@linkedin.com>

Reviewers: Prateek Maheshwari <pmaheshwari@apache.org>

Closes apache#1088 from dxichen/samza-2259
amahussein pushed a commit to amahussein/hadoop that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants