Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15136 )
Change subject: KUDU-1438: [java] Upgrade to Netty 4 ...................................................................... Patch Set 3: (19 comments) http://gerrit.cloudera.org:8080/#/c/15136/3/java/gradle/tests.gradle File java/gradle/tests.gradle: http://gerrit.cloudera.org:8080/#/c/15136/3/java/gradle/tests.gradle@62 PS3, Line 62: jvmArgs += "-Dio.netty.leakDetection.level=paranoid" > Do we want to enable this by default? Does it significantly slow down tests I don't think we want this by default. By default Netty uses "simple" mode and will instrument 1% of the reference counted objects. This is likely good enough considering most development won't touch the few Netty related classes. Paranoid mode is nice to validate while making Netty changes to ensure all reference counted objects are instrumented. http://gerrit.cloudera.org:8080/#/c/15136/3/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/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2705 PS3, Line 2705: * @deprecated the bossExecutor is no longer used > Maybe "is no longer used and will have no effect if provided"? Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2722 PS3, Line 2722: * @deprecated the bossExecutor is no longer used > See above. Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2725 PS3, Line 2725: public AsyncKuduClientBuilder nioExecutor(Executor workerExecutor) { > This is a new method, isn't it? Why add a brand new method just to deprecat Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2735 PS3, Line 2735: * @deprecated the bossExecutor is no longer used > See above. Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2739 PS3, Line 2739: LOG.info("bossCount is deprecated"); > Should also LOG.info in the other deprecated methods? I didn't want to add Logging to the method that takes both boss and work threads given it still has some effect. http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2744 PS3, Line 2744: * Set the maximum number of worker threads. > Since boss threads aren't a thing any more, should we expand on this defini Done 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.getBy Done 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 re Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java@107 PS3, Line 107: private final Bootstrap bootstrap; > Javadoc? Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java@432 PS3, Line 432: if (ctx != null && ctx.channel().isOpen()) { > Doc when is ctx null (at least when called from connect). Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java@803 PS3, Line 803: pipeline.addLast(NEGOTIATION_TIMEOUT_HANDLER, : new ReadTimeoutHandler(NEGOTIATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)); > What does it mean that we're no longer passing in our own HashedWheelTimer? The reason we passed the timer was documented in the AsyncKuduClient: // Share the timer with the socket channel factory so that it does not // create an internal timer with a non-daemon thread. It looks like the Netty 4 ReadTimeoutHandler doesn't create/use a timer at all anymore. Instead it uses scheduled tasks in the EventLoop. http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java: http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@524 PS3, Line 524: * @deprecated the bossExecutor is no longer used > Same comment as in AsyncKuduClient. Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@541 PS3, Line 541: @Deprecated > Same question as in AsyncKuduClient. Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@552 PS3, Line 552: * @deprecated the bossExecutor is no longer used > Same comment as in AsyncKuduClient. Done 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 msg can't be null given I check `!sslEmbedder.outboundMessages().isEmpty()` above. 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() exp Done http://gerrit.cloudera.org:8080/#/c/15136/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java@881 PS3, Line 881: and > an Done 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 Done -- 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: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 31 Jan 2020 19:29:53 +0000 Gerrit-HasComments: Yes
