-
Notifications
You must be signed in to change notification settings - Fork 7k
[core] Make Cancel Remote Task RPC Fault Tolerant #57965
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
[core] Make Cancel Remote Task RPC Fault Tolerant #57965
Conversation
Signed-off-by: joshlee <[email protected]>
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.
Code Review
This pull request makes the CancelRemoteTask RPC fault-tolerant and idempotent, which is a great improvement for robustness. The related renames from RemoteCancelTask to CancelRemoteTask improve consistency across the codebase. The addition of a Python test to verify the retry behavior is also a valuable contribution. The changes are well-implemented, but I found one issue in a fake client implementation that needs to be addressed.
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
edoakes
left a comment
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.
LGTM, but the naming is still a little misleading. Should we encode the ownership semantic? For example, CancelOwnedTask or CancelTaskOnOwner. I don't love either of those, but you get the gist.
|
(can rename in separate PR as desired) |
> Briefly describe what this PR accomplishes and why it's needed. Making Cancel Remote Task RPC idempotent and fault tolerant. Added a python test to verify retry behavior, no cpp test since it just calls CancelTask RPC so nothing to test. Also renamed uses of RemoteCancelTask to CancelRemoteTask since it should be consistent. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: xgui <[email protected]>
> Briefly describe what this PR accomplishes and why it's needed. Making Cancel Remote Task RPC idempotent and fault tolerant. Added a python test to verify retry behavior, no cpp test since it just calls CancelTask RPC so nothing to test. Also renamed uses of RemoteCancelTask to CancelRemoteTask since it should be consistent. --------- Signed-off-by: joshlee <[email protected]>
> Briefly describe what this PR accomplishes and why it's needed. Making Cancel Remote Task RPC idempotent and fault tolerant. Added a python test to verify retry behavior, no cpp test since it just calls CancelTask RPC so nothing to test. Also renamed uses of RemoteCancelTask to CancelRemoteTask since it should be consistent. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
> Briefly describe what this PR accomplishes and why it's needed. Making Cancel Remote Task RPC idempotent and fault tolerant. Added a python test to verify retry behavior, no cpp test since it just calls CancelTask RPC so nothing to test. Also renamed uses of RemoteCancelTask to CancelRemoteTask since it should be consistent. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
Description
Making Cancel Remote Task RPC idempotent and fault tolerant. Added a python test to verify retry behavior, no cpp test since it just calls CancelTask RPC so nothing to test. Also renamed uses of RemoteCancelTask to CancelRemoteTask since it should be consistent.