You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2023/06/12 08:36:16 UTC
[kudu-CR] [java] add buffer space limit for KuduSession
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 <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 12 Jun 2023 08:36:16 +0000
Gerrit-HasComments: Yes