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.

Sign up for GitHub

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

Jump to bottom

metadata: make Stringer implementation consistent #7355

Merged
merged 1 commit into from
Jun 27, 2024
Merged

metadata: make Stringer implementation consistent #7355

merged 1 commit into from
Jun 27, 2024

Conversation

dims
Copy link
Contributor

@dims dims commented Jun 26, 2024 •
edited
Loading

Please see the following kubernetes/etcd test scenario failure:
kubernetes/kubernetes#125688

and notes from @serathius here on why this is important for etcd watches to work:
#7137 (comment)

RELEASE NOTES:

  • metadata/metadata: ensure the String interface for pretty printing MD is consistent

@dims dims changed the title Make Metadata Stringer implementation deterministic metadata: make Stringer implementation deterministic Jun 26, 2024
@dims dims mentioned this pull request Jun 26, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Jun 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.36%. Comparing base (98e5dee) to head (460df21).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7355      +/-   ##
==========================================
- Coverage   81.47%   81.36%   -0.12%     
==========================================
  Files         348      348              
  Lines       26761    26754       -7     
==========================================
- Hits        21804    21768      -36     
- Misses       3772     3787      +15     
- Partials     1185     1199      +14
Files Coverage Δ
metadata/metadata.go 89.09% <100.00%> (-0.66%) ⬇️

... and 14 files with indirect coverage changes

easwars
easwars reviewed Jun 26, 2024
View reviewed changes
metadata/metadata.go Show resolved Hide resolved
metadata/metadata.go Outdated Show resolved Hide resolved
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jun 26, 2024
@easwars easwars added this to the 1.66 Release milestone Jun 26, 2024
@easwars
Copy link
Contributor

easwars commented Jun 26, 2024

Also, the associated unit test needs to be fixed.

@dims dims force-pushed the make-metadata-stringer-implementation-deterministic branch from d83679d to 5fab0b4 Compare June 26, 2024 19:27
@dims
Copy link
Contributor Author

dims commented Jun 26, 2024

thanks for the quick review @easwars !

@dims
Copy link
Contributor Author

dims commented Jun 26, 2024

Also, the associated unit test needs to be fixed.

@easwars added a couple more variations to the test case

easwars
easwars reviewed Jun 26, 2024
View reviewed changes
metadata/metadata_test.go Outdated Show resolved Hide resolved
easwars
easwars reviewed Jun 26, 2024
View reviewed changes
metadata/metadata.go Outdated Show resolved Hide resolved
@dims dims force-pushed the make-metadata-stringer-implementation-deterministic branch from 5fab0b4 to 5e527ea Compare June 26, 2024 20:12
@dims
Copy link
Contributor Author

dims commented Jun 26, 2024

@easwars Do i need to do something to trigger the tests? please let me know. ( checked https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md before asking :) )

@dfawley
Copy link
Member

dfawley commented Jun 26, 2024

I agree this is a regression that we should fix, because Go's fmt("%v", <map>) guarantees a consistent output.

But a small nit about terminology: let's use something like "consistent" or "consistently ordered" instead of "deterministic" to not imply that one can depend on the output being something well-defined that will be the same between releases.

dfawley
dfawley reviewed Jun 26, 2024
View reviewed changes
metadata/metadata.go Outdated Show resolved Hide resolved
@AnomalRoil
Copy link
Contributor

Sorry for that. I didn't mind the inconsistent output, following the logic from golang/protobuf#1121 (comment)

But it's true that printing a <map> in Go is consistent, I should have done that too.

easwars
easwars reviewed Jun 26, 2024
View reviewed changes
metadata/metadata.go Outdated Show resolved Hide resolved
@dims dims changed the title metadata: make Stringer implementation deterministic metadata: make Stringer implementation consistent Jun 27, 2024
@dims
Copy link
Contributor Author

dims commented Jun 27, 2024

But it's true that printing a <map> in Go is consistent, I should have done that too.

No worries at all @AnomalRoil - at least we caught it early enough.

@dims dims force-pushed the make-metadata-stringer-implementation-deterministic branch from 5e527ea to ce0c464 Compare June 27, 2024 15:32
@dims
Make Metadata Stringer implementation deterministic
460df21 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims dims force-pushed the make-metadata-stringer-implementation-deterministic branch from ce0c464 to 460df21 Compare June 27, 2024 17:20
@dims
Copy link
Contributor Author

dims commented Jun 27, 2024

@easwars @dfawley all outstanding comments were addressed. thanks!

@easwars easwars assigned dfawley and unassigned dims Jun 27, 2024
dfawley
dfawley approved these changes Jun 27, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@dfawley dfawley merged commit 6126383 into grpc:master Jun 27, 2024
13 checks passed
@dims
Copy link
Contributor Author

dims commented Jun 27, 2024

thanks for the quick engagement/reviews @easwars @dfawley

@dims
Copy link
Contributor Author

dims commented Jun 28, 2024

@easwars @dfawley i have a cherry pick PR open for v1.64.x #7366 .. However i see a v1.65.x branch as well, Should i open one against that too?

@dfawley
Copy link
Member

dfawley commented Jun 28, 2024

@dims - yes, please! we haven't yet released 1.65.0 but will be doing so very soon.

@dims
Copy link
Contributor Author

dims commented Jun 28, 2024

Done! filed #7367 for v1.65.x

@dims
Copy link
Contributor Author

dims commented Jun 28, 2024

we haven't yet released 1.65.0 but will be doing so very soon.

@dfawley is there a v1.64.1 as well soon? (pretty please!) will help us update OTEL (plus grpc!) to newer versions in k8s (had to revert that PR)

@dfawley
Copy link
Member

dfawley commented Jun 28, 2024

is there a v1.64.1 as well soon?

Yes, we will try to get that out next week.

@dims dims mentioned this pull request Jul 5, 2024
Merged
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AnomalRoil AnomalRoil AnomalRoil left review comments

@dfawley dfawley dfawley approved these changes

@easwars easwars easwars approved these changes

Assignees

@dfawley dfawley

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.66 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@dims @easwars @dfawley @AnomalRoil