Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19918 )
Change subject: [java] add buffer space limit for KuduSession ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java: http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@625 PS5, Line 625: activeBuffer.numOps() > 0 nit: !activeBuffer.isEmpty() http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@751 PS5, Line 751: (activeBufferOps + 1 >= mutationBufferMaxOps || : isAboveMaxSize(activeBufferSize + operation.getRow().size())) How about encapsulate a function like 'boolean needFlush(int activeBufferOps, long activeBufferSize)'? There are some duplicate code in L720, L729 and here. isAboveMaxSize() maybe possible to be removed then. http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@893 PS5, Line 893: nit: add some comments to clarify it's not the operations.size() http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@980 PS5, Line 980: .add("operations", operations.size()) How about output the 'operationSize' as well? http://gerrit.cloudera.org:8080/#/c/19918/5/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/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1912 PS5, Line 1912: Returns the in memory size of this row. To make it work well with javadoc, it would be great to format the comments, for example add a separated function comments and a keyword '@return'. http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1914 PS5, Line 1914: nit: remove the space. http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java File java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java: http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@98 PS5, Line 98: size nit: byte size http://gerrit.cloudera.org:8080/#/c/19918/5/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@100 PS5, Line 100: size nit: byte size -- To view, visit http://gerrit.cloudera.org:8080/19918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I312d47c98566f9405361d969a4b68b326bb3c4d9 Gerrit-Change-Number: 19918 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 08:36:16 +0000 Gerrit-HasComments: Yes
