Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/22491 )
Change subject: [java] code cleanup ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/22491/7//COMMIT_MSG Commit Message: PS7: > Could you share the information how you came up with this list of updates? Sure, I used Qodana[0] to try it's static code analysis. I'm not sure I'm terribly happy with the results it provided, many findings are kind of false-positive with some more context taken into consideration, these changes seemed like small easy low hanging fruits. I'm not sure about automation, Qodana has a paid cloud based solution where the results of these scans can be aggregated, not sure if they do free versions for open source projects though. Do we currently automatically run spotbugs during the jenkins builds or some other time? [0] https://www.jetbrains.com/qodana/ 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; > Did you consider the possibility that it was an intent to keep the result ' I honestly thought that this was just a small oversight, but you're right maybe there's some context I missed, though if that was the intention I would've expected at least a comment or mention in the documentation comment. Nevertheless I checked the commit[0] that introduced the frozen field but found no mention of this intention. The doc comment of the copy constructor: "Creates a new partial row by deep-copying the data-fields of the provided partial row." For me this means that every field should be copied, but maybe I'm not understanding the freezing mechanism of the PartialRows well enough. If by copying a PartialRow should become unfrozen, then I think we should add some comments here to make that clear. [0] https://gerrit.cloudera.org/c/1335/ http://gerrit.cloudera.org:8080/#/c/22491/7/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/7/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@131 PS7, Line 131: outputFile.toPath() > Is it going to be a NPE from here if close() is called twice? I'll test this to confirm. -- 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: 7 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: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Thu, 20 Feb 2025 14:27:27 +0000 Gerrit-HasComments: Yes
