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

client: enhance the function shouldRetryWatch and added unit test #14935

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Dec 12, 2022

client/v3/watch.go Show resolved Hide resolved
client/v3/watch.go Outdated Show resolved Hide resolved
client/v3/watch.go Outdated Show resolved Hide resolved
@ahrtr ahrtr force-pushed the minor_enhance_error_20221213 branch 2 times, most recently from 0845b67 to 4437ce1 Compare December 12, 2022 22:15
…gGRPCAuthOldRevision to cache gRPC error messages

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr force-pushed the minor_enhance_error_20221213 branch from 4437ce1 to d0e753c Compare December 12, 2022 23:29
@codecov-commenter
Copy link

Codecov Report

Merging #14935 (d0e753c) into main (ee9db72) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main   #14935      +/-   ##
==========================================
- Coverage   74.59%   74.57%   -0.02%     
==========================================
  Files         415      415              
  Lines       34338    34339       +1     
==========================================
- Hits        25613    25608       -5     
- Misses       7082     7084       +2     
- Partials     1643     1647       +4     
Flag Coverage Δ
all 74.57% <66.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/v3/watch.go 93.44% <66.66%> (-0.19%) ⬇️
client/v3/experimental/recipes/queue.go 58.62% <0.00%> (-6.90%) ⬇️
client/v3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
client/v3/concurrency/session.go 88.63% <0.00%> (-4.55%) ⬇️
server/storage/mvcc/kvstore_compaction.go 95.65% <0.00%> (-4.35%) ⬇️
server/etcdserver/raft.go 88.63% <0.00%> (-2.28%) ⬇️
server/etcdserver/api/v3rpc/watch.go 84.76% <0.00%> (-2.23%) ⬇️
server/etcdserver/api/rafthttp/msgappv2_codec.go 71.30% <0.00%> (-1.74%) ⬇️
server/storage/mvcc/kvstore.go 89.00% <0.00%> (-1.42%) ⬇️
server/etcdserver/v3_server.go 77.83% <0.00%> (-1.23%) ⬇️
... and 16 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

LGTM, thanks for enhancing it @ahrtr

@mitake mitake merged commit 6429c04 into etcd-io:main Dec 13, 2022
@mitake
Copy link
Contributor

mitake commented Dec 13, 2022

Probably I won't be able to have time for following the k8s side discussion until weekend... sorry for that. I think having something like reasonCode is a good idea though.

@ahrtr ahrtr deleted the minor_enhance_error_20221213 branch December 14, 2022 01:54
name: "invalid grpc error and not equal to ErrGRPCInvalidAuthToken or ErrGRPCAuthOldRevision",
msg: "whatever error message",
expectedRetry: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Now msg: "" is a special codepath - so probably worth having a testcase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually we need to revert the change and need to add a field something like ReasonCode in the WatchResponse. Or just rollback the change, do not fix it at all. What's your thought?

#14992

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

Successfully merging this pull request may close these issues.

5 participants