JAVA-3149: Support request cancellation in request throttler #1950
Conversation
core/revapi.json
Outdated
| { | ||
| "code": "java.method.addedToInterface", | ||
| "new": "method void com.datastax.oss.driver.api.core.session.throttling.RequestThrottler::signalCancel(com.datastax.oss.driver.api.core.session.throttling.Throttled)", | ||
| "justification": "JAVA-3149: Propagate cancellation of AsyncResultSet future to the throttler" |
There was a problem hiding this comment.
Similar to #1640 with ExecutionInfo#getExecutionProfile, could we should add a default implementation that in this case would essentially be a no-op?
There was a problem hiding this comment.
Yes, you are right. I though that nearly nobody declares custom request throttler. Reported also CASSANDRA-19908 to remove in 5.0.
| @@ -56,4 +56,10 @@ public interface RequestThrottler extends Closeable { | |||
| * perform time-based eviction on pending requests. | |||
| */ | |||
| void signalTimeout(@NonNull Throttled request); | |||
|
|
|||
| /** | |||
| * Signals that a request has been cancelled. This indicates to the throttler that another request | |||
There was a problem hiding this comment.
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 to signalTimeout 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. 👍
| @@ -124,6 +124,7 @@ protected CqlPrepareHandler( | |||
| try { | |||
| if (t instanceof CancellationException) { | |||
| cancelTimeout(); | |||
| context.getRequestThrottler().signalCancel(this); | |||
There was a problem hiding this comment.
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 CompletableFuture.cancel(..) cannot cancel a completed exception, so this should be fine.
There was a problem hiding this comment.
Correct, I think the same.
integration-tests/src/test/java/com/datastax/oss/driver/core/throttling/ThrottlingIT.java
Outdated
Show resolved
Hide resolved
| @@ -145,6 +145,22 @@ public void signalTimeout(@NonNull Throttled request) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public void signalCancel(@NonNull Throttled request) { | |||
There was a problem hiding this comment.
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).
lukasz-antoniak
force-pushed
the
JAVA-3149
branch
from
5a3585f to
8a462b1
Compare
There was a problem hiding this comment.
Looks great, thank you @lukasz-antoniak ! 👍 to merging and then we can turn to #1957
patch by Lukasz Antoniak; reviewed by Andy Tolbert and Chris Lohfink for JAVA-3149
lukasz-antoniak
force-pushed
the
JAVA-3149
branch
from
8a462b1 to
df12d0f
Compare
Fix JAVA-3149.