Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18183 )
Change subject: [java] KUDU-3240 Make connection negotiation timeout configurable for java client ...................................................................... Patch Set 2: (6 comments) Thank you for the patch! Overall looks good, just a few nits regarding the naming and outdated comments. http://gerrit.cloudera.org:8080/#/c/18183/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18183/2//COMMIT_MSG@9 PS2, Line 9: kudu nit: Kudu http://gerrit.cloudera.org:8080/#/c/18183/2//COMMIT_MSG@9 PS2, Line 9: java nit: Java http://gerrit.cloudera.org:8080/#/c/18183/2//COMMIT_MSG@10 PS2, Line 10: nit: remove the extra space http://gerrit.cloudera.org:8080/#/c/18183/2/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/18183/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2845 PS2, Line 2845: defaultNegotiationTimeoutMs In C++ client the corresponding method is called 'connection_negotiation_timeout()': it guess it make sense to keep the naming consistent between the two client implementations and call this new method 'connectionNegotiationTimeoutMs()' http://gerrit.cloudera.org:8080/#/c/18183/2/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/18183/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java@545 PS2, Line 545: defaultNegotiationTimeoutMs Same as in AsyncKuduClientBuilder: rename into 'connectionNegotiationTimeoutMs()' http://gerrit.cloudera.org:8080/#/c/18183/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java: http://gerrit.cloudera.org:8080/#/c/18183/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java@127 PS2, Line 127: * KUDU-1868: This tests that negotiation can time out on the client side. It passes if the : * hardcoded negotiation timeout is lowered to 500ms. In general it is hard to get it to work : * right because injecting latency to negotiation server side affects all client connections, : * including the harness's Java client, the kudu tool used to create the test cluster, and the : * other members of the test cluster. There isn't a way to configure the kudu tool's : * negotiation timeout within a Java test, presently. It seems this comments needs an update since the required functionality has been implemented in this patch. -- To view, visit http://gerrit.cloudera.org:8080/18183 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac1fbc2df3118cbab2f87751c5e21346478f72f9 Gerrit-Change-Number: 18183 Gerrit-PatchSet: 2 Gerrit-Owner: yejiabao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 31 Jan 2022 00:32:16 +0000 Gerrit-HasComments: Yes
