-
Notifications
You must be signed in to change notification settings - Fork 181
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
[FLINK-21308] Support delayed message cancellation #241
Conversation
4eb3180
to
2b2a8c9
Compare
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.
Looks very good @igalshilman!
I have one minor comment about optimizing (though perhaps overly doing it), depending on how common the scenario I described occurs. What do you think?
Other than that, this looks good to merge!
+ messageId | ||
+ " and timestamp " | ||
+ untilTimestamp | ||
+ ", but a message with the same id exists and with a timestamp " |
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.
nit: excessive "and"
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.
Same here: id
--> cancellation token
in the error messages.
We seem to refer to these as cancellation tokens on the API surface.
result.getOutgoingDelayCancellationsList()) { | ||
String token = delayCancellation.getCancellationToken(); | ||
if (!token.isEmpty()) { | ||
context.cancelDelayedMessage(delayCancellation.getCancellationToken()); |
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.
Assume the following scenario:
Within the same FromFunction
response from a remote function, there is a delayed message with cancellation token foobar
. At the same time, there is also a cancellation token for foobar
.
I'm wondering if we want to be diligent in this scenario and eagerly "cancel" out the delayed message, without going through a roundtrip of adding it to the buffer / index state, just to delete it almost imediately.
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.
I think this might even be a common case.
Say, for the same invocation batch with a given key, an earlier message triggers a delayed message that is cancellable. Maybe as a timer to purge state etc. in the absence of a follow up event.
However, within that same batch, the follow up event is processed and users immediately wants to cancel the delayed message. We can maybe optimize this scenario?
// so it would be re-enqueued into the delayedMessageBuffer. | ||
delayedMessagesBuffer.clearForTimestamp(triggerTimestamp); | ||
reductions.processEnvelopes(); | ||
public void onProcessingTime(InternalTimer<String, VoidNamespace> timer) { |
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.
👍
} catch (Exception e) { | ||
throw new RuntimeException( | ||
"Error accessing delayed message in state buffer for timestamp: " + timestamp, e); | ||
throw new IllegalStateException("Failed clearing a message with id " + token, e); |
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.
Maybe change id
in the error message to cancellation token
, to be consistent and not confuse users.
2b2a8c9
to
022bb70
Compare
@tzulitai Thanks for the review! I've squashed the previous history into a single commit, as the history wasn't that important. and the last 4 commits are the result of our conversation earlier today. |
Thanks @igalshilman! Merging this! |
This PR adds the ability to cancel delayed messages.
High level changes
This PR introduces the following methods in the embedded SDK, and the corresponding remote SDKs (language specific flavors)
Internal Changes
request reply protocol:
We attach the (optional)
cancelltion_token
to the delayed invocation in the request-reply protocol.In addition, a new response message:
State
We add an additional state handle (
delayed-message-index
) to keep track of the mapping between acancellation_token
and the absolute timestamp that this message needs to be dispatched at.These changes are then wired throughout the SDK and the runtime to make the magic happen.