client/v3: emit a single resolver update in EtcdManualResolver.Build - #22133
client/v3: emit a single resolver update in EtcdManualResolver.Build#22133Sunnatillo wants to merge 1 commit into
Conversation
|
Hi @Sunnatillo. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cc @ahrtr |
| resolver.ServerName | ||
| resolver.ServiceConfig |
There was a problem hiding this comment.
Thanks. Reworked it to add no experimental API - reverted the allowlist and rewrote the tests to compare whole resolver.State values
There was a problem hiding this comment.
@ahrtr do the changes after experimental api removal looks got to you?
236e128 to
199cf78
Compare
|
/cc @fuweid @serathius |
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 46 files with indirect coverage changes @@ Coverage Diff @@
## main #22133 +/- ##
==========================================
+ Coverage 69.66% 69.74% +0.07%
==========================================
Files 449 449
Lines 38177 38191 +14
==========================================
+ Hits 26597 26636 +39
+ Misses 10148 10133 -15
+ Partials 1432 1422 -10 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/retest |
|
Please also add a changelog item under See example https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.7.md#package-clientv3-1 |
ahrtr
left a comment
ahrtr
left a comment
There was a problem hiding this comment.
Thanks for your first contribution.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, Sunnatillo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| "google.golang.org/grpc/serviceconfig" | ||
| ) | ||
|
|
||
| // fakeClientConn records every state passed to UpdateState so tests can assert |
There was a problem hiding this comment.
Can we merge those tests first? Would like to understand what's the current state before we start changes.
There was a problem hiding this comment.
Tests are written for changed behavior. It expects Build() emits one update. In old behaviour it emits two update.
Do you want me to add tests for old behavior and change the test in this PR?
If we add the test in current format to main branch first it will fail.
|
Overall LGTM, left comments to remove AI slop comments and make change safer by merging tests first. |
Build pushed the endpoints and round_robin ServiceConfig via a follow-up updateState call after building the embedded manual.Resolver. gRPC saw an initial state without the ServiceConfig, then a second update with it, forcing a mid-connection balancer switch that canceled an in-flight dial and logged spurious "operation was canceled" warnings on every client. Seed the state via manual.Resolver.InitialState before Build so gRPC gets one update with both endpoints and the ServiceConfig. Extract a shared state() helper reused by updateState (SetEndpoints), and add resolver tests that assert Build emits exactly one update carrying both endpoints and the ServiceConfig. Assertions compare whole resolver.State values so no experimental gRPC API is introduced. Fixes etcd-io#21660 Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
199cf78 to
37e8925
Compare
|
Test failures are unrelated to the changes |
|
/retest |
@Sunnatillo can you run the test cases without product code change locally and show us that how it fails; afterwards apply the patch in resolver.go and prove that it resolves the test failure? Normally we add the test cases in the first commit, and add the patch in the second commit. (Personally I don't treat this as a hard requirement) |
|
@Sunnatillo: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
The document is the measured evaluation of the leader-aware client balancer (upstream proposal etcd-io/etcd#22268, base branch etcd-io/etcd#22133); the name says what it is.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
Build pushed the endpoints and round_robin ServiceConfig via a follow-up updateState call after building the embedded manual.Resolver. gRPC saw an initial state without the ServiceConfig, then a second update with it, forcing a mid-connection balancer switch that canceled an in-flight dial and logged spurious "operation was canceled" warnings on every client.
Seed the state via manual.Resolver.InitialState before Build so gRPC gets one update with both endpoints and the ServiceConfig. Add resolver tests and allowlist the experimental resolver fields they read.
Coauthored with: BootstrapperSBL
Fixes: #21660