Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16967 )
Change subject: KUDU-2612 keep-alive txn heartbeating for Java client ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveRequest.java: http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveRequest.java@42 PS2, Line 42: KuduTable fakeTable, nit: I know we use this elsewhere, but would it make it less surprising to call this masterTable or somesuch? http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveResponse.java: http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KeepTransactionAliveResponse.java@28 PS2, Line 28: * TODO(aserbin): clean up the list of parameters What don't we need? http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java: http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@578 PS2, Line 578: LOG.debug("starting keepalive heartbeating (txn ID {})", txnId); nit: I guess it'd be obvious from the absence of this, but I wonder if it's worth mentioning when there is no keepalive enabled. Also I wonder if it's worth mentioning the interval here, at least, for debugging purposes. OTOH, I understand it may be odd given the keepaliveMillis argument is different from the frequency that we're heartbeating. http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@602 PS2, Line 602: // TODO(aserbin): should we send next heartbeat sooner? Seems we're already doing this, given the keepaliveMillis/2? Or are you considering something even more frequent? http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java: http://gerrit.cloudera.org:8080/#/c/16967/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java@544 PS2, Line 544: getKeepaliveMillis(); nit: I know we've already done it in other tests, but since we're directly relying on it being set, perhaps assert that this is greater than 0? Same below -- To view, visit http://gerrit.cloudera.org:8080/16967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6326a5452223d8173b2da004bb5f3ab0c1e6ae4e Gerrit-Change-Number: 16967 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 21 Jan 2021 22:17:53 +0000 Gerrit-HasComments: Yes
