Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 )
Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/12338/4/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/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2353 PS4, Line 2353: void newTimeout(final TimerTask task, final long timeoutMs) { : try { : timer.newTimeout(task, timeoutMs, MILLISECONDS); : } catch (IllegalStateException e) { : // This can happen if the timer fires just before shutdown() : // is called from another thread, and due to how threads get : // scheduled we tried to call newTimeout() after timer.stop(). : LOG.warn("Failed to schedule timer. Ignore this if we're shutting down.", e); : } : } I think it'd be nice to avoid duplicating this in KuduRpc. Perhaps you can make this a static method, pass 'timer' into it, and use it from both classes? http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@72 PS4, Line 72: // TODO(wdberkeley): The fact we have to do this is a sign an Operation should not subclass : // KuduRpc. Nit: reformat into the Javadoc: /** * Reset the timeout.. * * TODO(wdberkeley): ... * * @param ... */ http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@a280 PS4, Line 280: Why did you remove this? Oh, I think it was for the change in AsyncKuduClient. Could you add an if statement there and restore this, instead? If that's the only hole in this invariant, I think the invariant is still worth keeping. http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@a374 PS4, Line 374: Why don't we want this in the toString() anymore? Was it not useful? http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@135 PS4, Line 135: Timeout timeout_task; Add a comment here, maybe explaining when this is null and when not null? http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java@35 PS4, Line 35: org.jboss.netty.util.Timer timer, No import for this case? http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java@33 PS4, Line 33: super(null, null, 0); Hmm, why is this a special case? No client-side deadline on these requests? http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java@104 PS4, Line 104: // TODO(wdberkeley): The fact we have to do this is a sign an Operation should not subclass : // KuduRpc. Nit: fold into the Javadoc. http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java@45 PS4, Line 45: super(null, null, 0); Another special case... http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/RpcTimeoutTask.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcTimeoutTask.java: http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/main/java/org/apache/kudu/client/RpcTimeoutTask.java@23 PS4, Line 23: final class RpcTimeoutTask implements TimerTask { There's a 1-1 relationship between this and a KuduRpc, right? Maybe it would be cleaner to define RpcTimeoutTask as an inner (non-static) class? That'll also mean you don't need to maintain the KuduRpc ref; it'd be de facto reachable. Also Javadoc. http://gerrit.cloudera.org:8080/#/c/12338/4/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/12338/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java@96 PS4, Line 96: KuduClient noRecvTimeoutClient = Declare this in a try-with-resources so that it's properly closed when you're done. http://gerrit.cloudera.org:8080/#/c/12338/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java@104 PS4, Line 104: // Scan with a short timeout. We want it to be long enough that client lookup on the master : // happens, but it's ok if it times out there, too. As long as the entire operation doesn't : // complete, the test passed. You could also do one scan without a timeout to cache the lookup result, then one scan with a timeout to prove that the timeout works. Or if you truly don't care whether the timeout happens during the lookup or during the scan, you might want to rewrite the "We want it to be long enough..." to be more clear. -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 05 Feb 2019 01:17:06 +0000 Gerrit-HasComments: Yes
