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

resolver/manual: allow calling UpdateState with an un-Built resolver #8150

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Mar 6, 2025

Also, unexport mutex-protected CC field, since it's impossible to safely access. And delete ReportError since it's just passed through to CC.

RELEASE NOTES: none

… unexport mutex-protected CC field
@dfawley dfawley added this to the 1.72 Release milestone Mar 6, 2025
@dfawley dfawley requested a review from arjan-bal March 6, 2025 17:40
Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.42%. Comparing base (5199327) to head (95a7081).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
resolver/manual/manual.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8150      +/-   ##
==========================================
+ Coverage   82.30%   82.42%   +0.12%     
==========================================
  Files         392      392              
  Lines       39140    39140              
==========================================
+ Hits        32213    32262      +49     
+ Misses       5605     5573      -32     
+ Partials     1322     1305      -17     
Files with missing lines Coverage Δ
internal/stubserver/stubserver.go 80.14% <100.00%> (ø)
test/parse_config.go 62.50% <100.00%> (ø)
resolver/manual/manual.go 92.68% <75.00%> (-7.32%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines -112 to -114
if r.CC == nil {
panic("cannot update state as channel has not exited IDLE state")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's clever, even if we call UpdateState before creating the resolver, it saves the state for later and returns. The saved state gets used when the resolver is built.

Does this make InitialState redundant? IIUC every caller of InitialState should now be able to call UpdateState.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's true, we probably don't need InitialState anymore. Maybe let's do a mini-migration after all these Dial->NewClient changes are done?

@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Mar 6, 2025
@dfawley dfawley merged commit 5668c66 into grpc:master Mar 6, 2025
15 checks passed
@dfawley dfawley deleted the manresunsafe branch March 6, 2025 21:40
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants