Skip to content
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

etcdserverpb: add "retry" flag to WatchResponse #15253

Closed
wants to merge 1 commit into from

Conversation

lavacat
Copy link

@lavacat lavacat commented Feb 7, 2023

Problem: client relies on string value of CancelReason to determine if watcher should be retried. This is too fragile. See #14992

Solution: add explicit retry flag

fixes: #15058

Signed-off-by: Bogdan Kanivets [email protected]

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Problem: client relies on string value of CancelReason to
determine if watcher should be retried. This is too fragile.
See etcd-io#14992

Solution: add explicit `retry` flag

fixes: etcd-io#15058

Signed-off-by: Bogdan Kanivets <[email protected]>
@@ -834,6 +834,9 @@ message WatchResponse {
// framgment is true if large watch response was split over multiple responses.
bool fragment = 7 [(versionpb.etcd_version_field)="3.4"];

// retry is true if watcher should be retried on cancel.
bool retry = 8 [(versionpb.etcd_version_field)="3.5"];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll backport to 3.5, so setting version 3.5 here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No backports of fields!

@lavacat
Copy link
Author

lavacat commented Feb 8, 2023

This code is covered by TestV3AuthWatchAndTokenExpire integration test.
Looked at adding unit test to v3rpc/watch_test.go, but it requires setting up serverWatchStream.recvLoop.

@lavacat lavacat marked this pull request as ready for review February 8, 2023 00:45
@serathius
Copy link
Member

This is a small change but with a big impact on important API semantics. Please start with creating issue discussing problem you want to solve and why solution you propose is worth adopting.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to first discuss different options before picking a solution.

@lavacat
Copy link
Author

lavacat commented Feb 8, 2023

@serathius do you mind continuing discussion in #15058? Here is my comment explainig retry. Other folks commented about this, so don't want to loose context.

@stale
Copy link

stale bot commented May 21, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 21, 2023
@stale stale bot closed this Jun 18, 2023
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After having consensus about the approach, I think we should update the changelog for describing the new field (or changes for other approach): https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.6.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Add a Code based member for cancel reason in WatchResponse
3 participants