Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16969 )
Change subject: [java] address warnings from the SonarLint tool ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/16969/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java: http://gerrit.cloudera.org:8080/#/c/16969/1/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java@118 PS1, Line 118: return length == that.length; > I am not sure I prefer this syntax to the existing syntax. Do you think it' SonarLint for prefers this form, I guess because of less code generated? I haven't checked how it looks like from the JVM point, but I guess the benefits are non-material practically. I agree we should keep the original version if thinking about better readability. http://gerrit.cloudera.org:8080/#/c/16969/1/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/16969/1/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@51 PS1, Line 51: int numRows) { > I think reuseRowResult was kept around for compatibility concerns. It would Thank you for pointing at this. One related change I found was: https://gerrit.cloudera.org/#/c/15983/ By the time of that change, it was a private constructor, so compatibility issues were not in play, as I understand. Then the constructor was made protected in https://gerrit.cloudera.org/#/c/15983/ It's now called from package private constructors of ColumnarRowResultIterator and RowwiseRowResultIterator (both are marked with @InterfaceAudience.Private). As of now, even if this class is marked with InterfaceAudience.Public, it's not possible to call this protected constructor outside of the package, right? As per https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html and https://stackoverflow.com/questions/5150748/protected-constructor-and-accessibility With that, I don't see compatibility concerns here. Please let me know if I'm missing something. http://gerrit.cloudera.org:8080/#/c/16969/1/java/kudu-client/src/main/java/org/apache/kudu/util/AsyncUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/AsyncUtil.java: http://gerrit.cloudera.org:8080/#/c/16969/1/java/kudu-client/src/main/java/org/apache/kudu/util/AsyncUtil.java@75 PS1, Line 75: private AsyncUtil() { > Should this go at the top of the class? Here and elsewhere. I put this in the very end of the file to match what we already have in the StringUtil.java file: https://github.com/apache/kudu/blob/202bcae7f12ca0d7ac43e0a07e57f3236c8f968d/java/kudu-client/src/main/java/org/apache/kudu/util/StringUtil.java#L88-L90 I guess the idea is to put not-so-relevant methods into the end of the file. -- To view, visit http://gerrit.cloudera.org:8080/16969 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I306575b370fb69a54fdc24b9d770a99291828ef7 Gerrit-Change-Number: 16969 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Comment-Date: Tue, 26 Jan 2021 23:29:42 +0000 Gerrit-HasComments: Yes
