Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15136 )
Change subject: KUDU-1438: [java] Upgrade to Netty 4 ...................................................................... Patch Set 3: (14 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? 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"? You could also remove all or most of the Javadoc, since none of it is relevant any more. 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. 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 deprecate it? 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. 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? 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 definition to explain what a 'worker' thread does? 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? 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). 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? Does Netty instantiate a separate one for internal use? Can we get it to reuse the one we create for ourselves? 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. 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. 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. 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@881 PS3, Line 881: and an -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 30 Jan 2020 23:37:53 +0000 Gerrit-HasComments: Yes
