Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22491 )
Change subject: [java] code cleanup ...................................................................... Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/22491/7//COMMIT_MSG Commit Message: PS7: > Sure, I used Qodana[0] to try it's static code analysis. I'm not sure I'm t Thanks for the reference. Yes, the pre-commit pipeline runs some checks. As you could see from build-support/jenkins/build-and-test.sh, we do run the 'check' Gradle task -- it includes spotbugs, checkstyle, animalsniffer, etc. http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2773 PS9, Line 2773: @SuppressWarnings("ReferenceEquality") If you changed the comparison from reference to value, this suppression might be removed, right? However, I'd encourage you to check the history behind this code to understand why comparing references here is the desired behavior. I suspect we don't need to switch to value comparison which might be slower. You could start with changelist c5521d99b, and dig a bit deeper. Thank you! http://gerrit.cloudera.org:8080/#/c/22491/7/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java: http://gerrit.cloudera.org:8080/#/c/22491/7/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@114 PS7, Line 114: this.frozen = row.frozen; > If by copying a PartialRow should become unfrozen, then I think we should add > some comments here to make that clear. Indeed -- if that was used as some 'feature', some tests would be broken with this patch. But I didn't see anything that breaks due to this update. http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-client/src/main/java/org/apache/kudu/client/Statistics.java File java/kudu-client/src/main/java/org/apache/kudu/client/Statistics.java: http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-client/src/main/java/org/apache/kudu/client/Statistics.java@145 PS9, Line 145: // This cast forces the compiler to invoke Map.keySet() rather than : // ConcurrentHashMap's override, which is critical because when this code : // is built with JDK8, ConcurrentHashMap.keySet() introduces a dependency : // on a Java 8 only API. : // : // Note: an alternative would be to always access stsMap as a Map, but that : // just moves the problem to the putIfAbsent() call in getTabletStatistics(), : // which is only a Map method in Java 8. : // : // See KUDU-2188 for details. : tablets.addAll(stsMap.keySet()); Could you double check that since this code changed, the comment now corresponds to what's going on with the updated code? Briefly looking here, I'm not sure about this. From the other side, since we switched to requiring Java 8 with Kudu 1.10, maybe it makes sense to re-visit this code from that perspective while you are at this? http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java: http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@93 PS9, Line 93: Files.delete(outputFile.toPath()); If you switched this to using newer NIO.2 API, maybe switch to the new API the call site where the temp file is created as well? http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-test-utils/src/main/java/org/apache/kudu/test/TempDirUtils.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/TempDirUtils.java: http://gerrit.cloudera.org:8080/#/c/22491/9/java/kudu-test-utils/src/main/java/org/apache/kudu/test/TempDirUtils.java@98 PS9, Line 98: nit: the indent seems to be incorrect (it seems 2 extra spaces were added?) -- To view, visit http://gerrit.cloudera.org:8080/22491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf7ffb0689f4c190fd585e73687b9c6c668890 Gerrit-Change-Number: 22491 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 01 Apr 2025 02:44:29 +0000 Gerrit-HasComments: Yes
