-
Notifications
You must be signed in to change notification settings - Fork 885
JAVA-3149: Support request cancellation in request throttler #1950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,7 @@ protected CqlPrepareHandler( | |
try { | ||
if (t instanceof CancellationException) { | ||
cancelTimeout(); | ||
context.getRequestThrottler().signalCancel(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thought I immediately had was, "is it possible that we might signal to the throttler twice for the same request?". It doesn't appear to be possible from what I can tell. It looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, I think the same. |
||
} | ||
} catch (Throwable t2) { | ||
Loggers.warnWithException(LOG, "[{}] Uncaught exception", logPrefix, t2); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,22 @@ public void signalTimeout(@NonNull Throttled request) { | |
} | ||
} | ||
|
||
@Override | ||
public void signalCancel(@NonNull Throttled request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code for signalTimeout/signalCancel is functionally the same except for the log, but it's probably fine this way (complexity of method is low enough that refactoring into a common method is probably not worth it). |
||
lock.lock(); | ||
try { | ||
if (!closed) { | ||
if (queue.remove(request)) { // The request has been cancelled before it was active | ||
LOG.trace("[{}] Removing cancelled request from the queue", logPrefix); | ||
} else { | ||
onRequestDone(); | ||
} | ||
} | ||
} finally { | ||
lock.unlock(); | ||
} | ||
} | ||
|
||
@SuppressWarnings("GuardedBy") // this method is only called with the lock held | ||
private void onRequestDone() { | ||
assert lock.isHeldByCurrentThread(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a little bit to come around to why we couldn't just reuse an existing api method (much like the originator of the JIRA re-used
signalTimeout
), but I think i agree that it makes sense that we add a new API method.Calling
future.cancel()
does not actually cancel an inflight query, which I suppose is a similar story tosignalTimeout
in that C* nodes may still not be working on that query.I think it's useful to make this a separate method so throttling implementations can make their own decision about how to handle cancel. 👍