You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/11/26 22:12:30 UTC
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14801
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
[java] Enable and fix low SpotBugs warnings
This patch fixes the low SpotBugs issues along with some other minor
cleanup spotted along the way. Either the SpotBugs issues were fixed or
they were added to the excludeFilter as a special case.
Follow on patches will enforce SpotBugs in the pre-commit build.
Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle/quality.gradle
M java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupIO.scala
M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/ExportCsvMapper.java
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java
32 files changed, 374 insertions(+), 208 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/14801/1
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
[java] Enable and fix low SpotBugs warnings
This patch fixes the low SpotBugs issues along with some other minor
cleanup spotted along the way. Either the SpotBugs issues were fixed or
they were added to the excludeFilter as a special case.
Follow on patches will enforce SpotBugs in the pre-commit build.
Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Reviewed-on: http://gerrit.cloudera.org:8080/14801
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle/quality.gradle
M java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupIO.scala
M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/ExportCsvMapper.java
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java
35 files changed, 395 insertions(+), 222 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS3, Line 437: return new X509Certificate[0];
> https://stackoverflow.com/questions/42294136/in-x509trustmanager-what-are-m
Yep, I think it's correct. As for the authoritative docs on that, there is https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/X509TrustManager.html#getAcceptedIssuers--
So, the methods should not return null, but an empty array instead.
I think the important point is that the overridden method is not used anywhere, but it was necessary to override it because of the implement-an-interface requirements.
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 28 Nov 2019 06:56:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116
PS3, Line 116: if (!hasNext()) {
: throw new NoSuchElementException();
: }
What would this do previously? Throw a different exception somewhere else?
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS3, Line 437: return new X509Certificate[0];
This seems like a real functional change. What motivated it?
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 20:16:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
File java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java@739
PS2, Line 739: context.write(new Text(keyString), new Text(refsList.toString()));
Isn't it a functional change? Does test pass after this has been changed?
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java@69
PS2, Line 69: transient
Why is this important? Is this exception is ever serialized?
It would be nice to add a comment if there is some non-trivial catch about this.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116
PS2, Line 116: if (!hasNext()) {
: throw new NoSuchElementException();
: }
Good catch! It seems we don't have much test coverage for this method?
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS2, Line 437: X509Certificate[0];
Interesting: I guess this method is not used at all, but it should not return null (per documentation). I'm curious whether the SpotBugs spotted that because of some sort of specific rule for X509TrustManager, NotNull annotation (I could not find one) or that's something generic for methods returning arrays?
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 08:34:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS3, Line 437: return new X509Certificate[0];
> The X509TrustManager interface documentation states that this should retur
https://stackoverflow.com/questions/42294136/in-x509trustmanager-what-are-method-getacceptedissuers-used-for was also helpful, assuming it's correct.
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 23:14:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116
PS3, Line 116: if (!hasNext()) {
: throw new NoSuchElementException();
: }
> What would this do previously? Throw a different exception somewhere else?
I think various different exceptions could be thrown depending on the state.
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS3, Line 437: return new X509Certificate[0];
> This seems like a real functional change. What motivated it?
The X509TrustManager interface documentation states that this should return "a non-null (possibly empty) array of acceptable CA issuer certificates."
The motivation was a spotbugs report of "PZLA_PREFER_ZERO_LENGTH_ARRAYS: Consider returning a zero length array rather than null".
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 20:52:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 2: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
File java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java@739
PS2, Line 739: context.write(new Text(keyString), new Text(refsList.toString()));
> No, refsList is guaranteed not to be null because it's initialized just abo
Indeed.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java@69
PS2, Line 69: transient
> Exceptions are marked as serializable, but these fields aren't serialized.
Yep, that makes sense if we want to get rid of warnings.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116
PS2, Line 116: if (!hasNext()) {
: throw new NoSuchElementException();
: }
> We have decent coverage, but always called hasNext() before next() or used
I meant we don't have tests to verify the iterator-related contracts for this next() method.
Anyway, adding such a test is beyond the scope of this patch.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS2, Line 437: X509Certificate[0];
> SpotBugs reported "PZLA_PREFER_ZERO_LENGTH_ARRAYS: Consider returning a zer
Thank you for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 18:10:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14801
to look at the new patch set (#2).
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
[java] Enable and fix low SpotBugs warnings
This patch fixes the low SpotBugs issues along with some other minor
cleanup spotted along the way. Either the SpotBugs issues were fixed or
they were added to the excludeFilter as a special case.
Follow on patches will enforce SpotBugs in the pre-commit build.
Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle/quality.gradle
M java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupIO.scala
M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/ExportCsvMapper.java
M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/JarFinder.java
M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/FakeDNS.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java
35 files changed, 395 insertions(+), 222 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/14801/2
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] Enable and fix low SpotBugs warnings
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java
File java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java@739
PS2, Line 739: context.write(new Text(keyString), new Text(refsList.toString()));
> Isn't it a functional change? Does test pass after this has been changed?
No, refsList is guaranteed not to be null because it's initialized just above at line 716.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java@69
PS2, Line 69: transient
> Why is this important? Is this exception is ever serialized?
Exceptions are marked as serializable, but these fields aren't serialized. We don't really care about serializing this exception, so I mark them as transient.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116
PS2, Line 116: if (!hasNext()) {
: throw new NoSuchElementException();
: }
> Good catch! It seems we don't have much test coverage for this method?
We have decent coverage, but always called hasNext() before next() or used a forEach loop.
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:
http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437
PS2, Line 437: X509Certificate[0];
> Interesting: I guess this method is not used at all, but it should not retu
SpotBugs reported "PZLA_PREFER_ZERO_LENGTH_ARRAYS: Consider returning a zero length array rather than null". Preferring empty collections over null is somewhat standard in Java.
I checked the Javadoc to verify the expectation and made the change.
--
To view, visit http://gerrit.cloudera.org:8080/14801
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440
Gerrit-Change-Number: 14801
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Nov 2019 17:13:42 +0000
Gerrit-HasComments: Yes