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 2020/02/10 15:12:05 UTC

[kudu-CR] [java] Fix various build warnings 

Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15191


Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
10 files changed, 68 insertions(+), 42 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15191/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>

[kudu-CR] [java] Fix various build 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/15191 )

Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Reviewed-on: http://gerrit.cloudera.org:8080/15191
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
M java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java
17 files changed, 136 insertions(+), 91 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved
  Alexey Serbin: Looks good to me, but someone else must approve

-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15191/5/java/gradle.properties
File java/gradle.properties:

http://gerrit.cloudera.org:8080/#/c/15191/5/java/gradle.properties@52
PS5, Line 52: summary
> It is too early to set it to 'fail' yet?
This is for Gradle deprecations (currently for Gradle 7.0) so I don't think we would ever set it to fail. 

For checkstyle, spotbugs, and errorprone I am working on adding it to the jenkins lint job.



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Feb 2020 14:17:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java@161
PS3, Line 161: @Override
             :   public boolean equals(Object o) {
             :     return super.equals(o);
             :   }
             : 
             :   @Override
             :   public int hashCode() {
             :     return super.hashCode();
             :   }
By default, won't the parent's method be invoked anyways?


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@131
PS3, Line 131:  new InetAddress[0];
Array of size 0. Why this change? Prevents NPE?
Need to update Javadoc comment.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@96
PS3, Line 96:               String.format("unable to receive message, expected (%d) bytes " +
            :                       "but read (%d) bytes", size, read));
Wasn't the earlier formatting correct?


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java@36
PS3, Line 36: public abstract class ProtocolHandler<I extends Message,
            :                                       O extends Message> 
I find earlier Request/Response generic types better for readability than I/O.



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Feb 2020 21:29:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:

http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@59
PS4, Line 59: T
lower case


http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@60
PS4, Line 60: cause
", causing"



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Feb 2020 23:33:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao, Bankim Bhavsar, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15191

to look at the new patch set (#4).

Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
M java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java
18 files changed, 137 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15191/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15191

to look at the new patch set (#2).

Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
9 files changed, 67 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15191/2
-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [java] Fix various build warnings

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Feb 2020 05:25:30 +0000
Gerrit-HasComments: No

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@115
PS3, Line 115:             <!-- Often tests testing exceptions have "exception" in the name. -->
> Seems easy enough to address (later).
The name made sense (PrintStreamWithIOException) so I didn't see a reason to change it.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@239
PS3, Line 239:         <!-- Reference equality is intended here. -->
> Not clear how the three <Bug> elements have to do with this.
Bad copy paste.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@242
PS3, Line 242:             <Bug pattern="BIT_ADD_OF_SIGNED_BYTE" />
             :             <Bug pattern="SF_SWITCH_FALLTHROUGH" />
             :             <Bug pattern="SF_SWITCH_NO_DEFAULT" />
> You modified the code to disable checkstyle though; does that mean this is 
This is the spotbugs file. Both tools flagged it.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java@161
PS3, Line 161: @Override
             :   public boolean equals(Object o) {
             :     return super.equals(o);
             :   }
             : 
             :   @Override
             :   public int hashCode() {
             :     return super.hashCode();
             :   }
> By default, won't the parent's method be invoked anyways?
Yes, but that doesn't satisfy spotbugs. We still want the parent's behavior so I override calling super.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@131
PS3, Line 131:  new InetAddress[0];
> Yeah I agree this looks fishy. Is it possible for getAllByName() to return 
I will just undo and add a rule exclusion.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@96
PS3, Line 96:               String.format("unable to receive message, expected (%d) bytes " +
            :                       "but read (%d) bytes", size, read));
> Wasn't the earlier formatting correct?
Done


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java@36
PS3, Line 36: public abstract class ProtocolHandler<I extends Message,
            :                                       O extends Message> 
> I find earlier Request/Response generic types better for readability than I
I think the issue is that it makes it less clear that they are types and not variables. 

The checkstlye rule for this is described here: https://checkstyle.sourceforge.io/config_naming.html#ClassTypeParameterName

I will do RequestT and ResponseT since that conforms to the rule.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@58
PS3, Line 58:       // Exit the program with a nonzero status code, by throwing a runtime exception.
            :       // if unexpected exception(s) are thrown by the reader or writer tasks.
> Comment needs to be rewritten.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Feb 2020 23:02:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml
File java/config/spotbugs/excludeFilter.xml:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@115
PS3, Line 115:             <!-- Often tests testing exceptions have "exception" in the name. -->
Seems easy enough to address (later).


http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@239
PS3, Line 239:         <!-- Reference equality is intended here. -->
Not clear how the three <Bug> elements have to do with this.


http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@242
PS3, Line 242:             <Bug pattern="BIT_ADD_OF_SIGNED_BYTE" />
             :             <Bug pattern="SF_SWITCH_FALLTHROUGH" />
             :             <Bug pattern="SF_SWITCH_NO_DEFAULT" />
You modified the code to disable checkstyle though; does that mean this is no longer needed?


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@131
PS3, Line 131:  new InetAddress[0];
> Array of size 0. Why this change? Prevents NPE?
Yeah I agree this looks fishy. Is it possible for getAllByName() to return an _empty list_? If so, then we should preserve the returning of null here, to differentiate between "no IP addresses" and "exception while resolving the name".


http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:

http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@58
PS3, Line 58:       // Exit the program with a nonzero status code, by throwing a runtime exception.
            :       // if unexpected exception(s) are thrown by the reader or writer tasks.
Comment needs to be rewritten.



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Feb 2020 21:47:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15191

to look at the new patch set (#3).

Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
M java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java
17 files changed, 125 insertions(+), 90 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15191/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [java] Fix various build warnings

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 4: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 10 Feb 2020 23:44:44 +0000
Gerrit-HasComments: No

[kudu-CR] [java] Fix various build warnings

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15191/5/java/gradle.properties
File java/gradle.properties:

http://gerrit.cloudera.org:8080/#/c/15191/5/java/gradle.properties@52
PS5, Line 52: summary
It is too early to set it to 'fail' yet?



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Feb 2020 05:48:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )

Change subject: [java] Fix various build warnings
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java:

http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@59
PS4, Line 59: T
> lower case
Done


http://gerrit.cloudera.org:8080/#/c/15191/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@60
PS4, Line 60: cause
> ", causing"
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Feb 2020 01:09:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] [java] Fix various build warnings

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao, Bankim Bhavsar, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15191

to look at the new patch set (#5).

Change subject: [java] Fix various build warnings
......................................................................

[java] Fix various build warnings

This patches some small checkstyle, spotbugs, and gradle
build warnings.

Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
---
M java/config/spotbugs/excludeFilter.xml
M java/gradle.properties
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/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java
M java/kudu-client/src/main/java/org/apache/kudu/util/HashUtil.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M java/kudu-client/src/test/java/org/apache/kudu/util/TestFashHash.java
M java/kudu-subprocess-echo/src/test/java/org/apache/kudu/subprocess/echo/TestEchoSubprocess.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java
M java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java
17 files changed, 136 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/15191/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15191
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b
Gerrit-Change-Number: 15191
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)