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

metadata: make Stringer implementation consistent #7355

Merged

Conversation

dims
Copy link
Contributor

@dims dims commented Jun 26, 2024

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
Copy link

codecov bot commented Jun 26, 2024

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

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

metadata/metadata_test.go Outdated Show resolved Hide resolved
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.

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.

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
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
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.

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

Successfully merging this pull request may close these issues.

4 participants