Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15136 )
Change subject: KUDU-1438: [java] Upgrade to Netty 4 ...................................................................... Patch Set 3: (5 comments) a few nits http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java File java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@987 PS3, Line 987: int readerIndex = buf.readerIndex(); nit: maybe, avoid adding a variable and call buf.readerIndex() as buf.getBytes(buf.readerIndex(), array) ? http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java@169 PS3, Line 169: ReferenceCountUtil.retain(msg); Could you add a comment explaining why it's necessary to retain an extra reference here? http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@628 PS3, Line 628: msg Can 'msg' be null and whether it's necessary to handle it some special way in that case? http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@630 PS3, Line 630: msg.release(); Could you add a comment explaining why it's necessary to call release() explicitly here? http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthTokenReacquire.java@176 PS3, Line 176: : %s maybe, there is no need to duplicate this piece of the exception info twice here because it will be in the stack trace as well? -- To view, visit http://gerrit.cloudera.org:8080/15136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic75bd15a982187039ff2e1510af9390d304f7626 Gerrit-Change-Number: 15136 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 31 Jan 2020 05:38:55 +0000 Gerrit-HasComments: Yes
