Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1542: in some cases apply will hang. ......................................................................
Patch Set 4: > (3 comments) > > Let me make sure I understand what's happening here. > > The problem: with AUTO_FLUSH_SYNC, apply() can loop forever if each > retry throws a PleaseThrottleException. It's not quite looping > forever though; it's just that there's nothing stopping this > operation from hitting "bad luck" in every call to apply() and > getting repeatedly starved by other operations. Is that right? > > If that's accurate, I don't think this is the right approach to > fixing the problem. Instead, we should prioritize this operation > (the one that saw a PTE) such that after it waits on the PTE's > deferred and retries, it cannot be starved by any operation that > comes afterward. The intent of KuduSession is that it's not thread-safe, so if you're waiting on a Deferred, when it clears you should be the only one applying. > > I'm also not sure why we'd want to place an upper bound on the > number of retry attempts. Ultimately, users only care about > timeouts and deadlines, as they correspond to SLAs. Put another > way, an upper bound on retries doesn't correspond to an upper bound > on wall clock time. Calculating a deadline using getTimeoutMillis() > when entering apply() and using that as a determinant for when to > break out of the apply() loop seems better, though avoiding > starvation altogether using the above is probably ideal. I agree we should interrogate the RPC to see if it has timed out, that what I proposed yesterday too. There's no need to add a new "max retries" concept on top of what we already have. The only caveat is that we don't always track timeouts on individual RPCs: * Timeouts are handled differently depending on the flush mode. * With AUTO_FLUSH_SYNC, the timeout is set on each apply()'d operation. * With AUTO_FLUSH_BACKGROUND and MANUAL_FLUSH, the timeout is assigned to a whole batch of * operations upon flush()'ing. It means that in a situation with a timeout of 500ms and a flush * interval of 1000ms, an operation can be outstanding for up to 1500ms before being timed out. -- To view, visit http://gerrit.cloudera.org:8080/3787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iacd33cdc5316e294e613d1b2273ef12e6b1cf687 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: song bruce zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: song bruce zhang <[email protected]> Gerrit-HasComments: No
