peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

AnomalRoil · 2024-04-17T16:01:01Z

Effectively allowing to print contextes containing a Peer properly.
Instead of printing with %v as:

context.Background.WithValue(type peer.peerKey, val <not Stringer>)

contexts containing a peer will now print as:

context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', AuthInfo: 'testAuthInfo'})

This is also satisfying Stringer interface for Metadata, allowing to print them easily too. Since Metadata are basically a map, notice that String output is not stable. I could change it to print it in alphabetical order if you prefer.

RELEASE NOTES:

peer/peer: implement the Stringer interface for pretty printing Peer
metadata/metadata: implement the Stringer interface for pretty printing MD

linux-foundation-easycla · 2024-04-17T16:01:06Z

The committers listed above are authorized under a signed CLA.

✅ login: AnomalRoil / name: AnomalRoil (83118ab, 985347e, 20ee25c, 172603c, d36bfd5, c908bad, edfd323, 067b07e, e482bcc)

peer/peer.go

metadata/metadata.go

peer/peer_test.go

balancer_wrapper.go

Effectively allowing to print context containing Peers properly Also enabling logging of Metadata in an easier way RELEASE NOTES: - peer/peer: implement the Stringer interface for pretty-printing Peers - metadata/metadata: implement the Stringer interface for pretty printing metadata Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

…fication of which SubConn is used Protect access to curAddr with ac.mutex Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

…n identification of which SubConn is used" This reverts commit 001f394. Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

arvindbr8

The change overall looks good to me. I took a pass at it. please take a look

metadata/metadata.go

metadata/metadata_test.go

peer/peer.go

peer/peer_test.go

metadata/metadata.go

arvindbr8

The change overall looks good to me. I took a pass at it. please take a look

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

arvindbr8

Thanks, I've responded to our conversations here. Please take a look

metadata/metadata.go

peer/peer.go

metadata/metadata.go

metadata/metadata_test.go

peer/peer_test.go

metadata/metadata_test.go

Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

arvindbr8 · 2024-05-02T17:23:14Z

peer/peer_test.go

+		want string
+	}{
+		{
+			name: "+Addr-LocalAddr+ValidAuth",


Just FYI, go test std library already does something similar by replacing \ss with _s.

I was trying to convey information about whether the test case included + or didn't include - that specific field.

The use of + and - to convey the presence or absence of certain fields within the Peer isn't very readable. How about the following:

withAddrAndValidAuth

withAddrLocalAddrAndValidAuth

withAddrAndEmptyAuth

withAddrLocalAddrAndEmptyAuth

nilPeer

The above names in the test output are more readable I feel.

arvindbr8 · 2024-05-02T17:24:27Z

Per policy we need one more review before we can check this in. I will ping a co-maintainer to take a look at this.

zasweq

LGTM. Thank you for the contribution.

serathius · 2024-06-26T18:32:35Z

Making String() non-deterministic broke etcd stream selection mechanism.

dims · 2024-06-26T18:45:07Z

@serathius @zasweq the following patch to make the string deterministic solves our issue:

diff --git a/vendor/google.golang.org/grpc/metadata/metadata.go b/vendor/google.golang.org/grpc/metadata/metadata.go
index 6c01a9b359cc..ec1055f6ccf0 100644
--- a/vendor/google.golang.org/grpc/metadata/metadata.go
+++ b/vendor/google.golang.org/grpc/metadata/metadata.go
@@ -24,6 +24,7 @@ package metadata // import "google.golang.org/grpc/metadata"
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 
 	"google.golang.org/grpc/internal"
@@ -95,11 +96,16 @@ func Pairs(kv ...string) MD {
 func (md MD) String() string {
 	var sb strings.Builder
 	fmt.Fprintf(&sb, "MD{")
-	for k, v := range md {
+	keys := make([]string, 0, len(md))
+	for k := range md {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, k := range keys {
 		if sb.Len() > 3 {
 			fmt.Fprintf(&sb, ", ")
 		}
-		fmt.Fprintf(&sb, "%s=[%s]", k, strings.Join(v, ", "))
+		fmt.Fprintf(&sb, "%s=[%s]", k, strings.Join(md[k], ", "))
 	}
 	fmt.Fprintf(&sb, "}")
 	return sb.String()

serathius · 2024-06-26T18:53:03Z

Context why deterministic String() is important for etcd.

Etcd uses grpc metadata to change properties of the bi-directional watch stream. Example of that is WithRequireLeader() which will ask server to break the stream if leader is no longer available in the cluster. As client can make multiple watch requests within a bi-directional stream, we need to create a separate stream for clients that provide different metadata in the request context. Etcd client get the identifiers of stream by formatting the context metadata, so it's crucial to have deterministic stringer to properly reuse the streams.

Code: https://github.com/etcd-io/etcd/blob/21e5876f7f0539509c277b4c2a3ba1b1599b1721/client/v3/watch.go#L1038-L1043

easwars · 2024-06-26T18:47:54Z

metadata/metadata.go

@@ -90,6 +90,21 @@ func Pairs(kv ...string) MD {
 	return md
 }

+// String implements the Stringer interface for pretty-printing a MD.
+// Ordering of the values is non-deterministic as it ranges over a map.


What do you think about sorting the keys before generating the string representation and ensuring that the output is deterministic. This will also make the unit test much simpler than what it is currently.

easwars · 2024-06-26T18:48:38Z

metadata/metadata.go

+	var sb strings.Builder
+	fmt.Fprintf(&sb, "MD{")
+	for k, v := range md {
+		if sb.Len() > 3 {


How about defining a bool first and using that as the conditional here, instead of this magic check which took me sometime to understand.

easwars · 2024-06-26T18:52:42Z

peer/peer_test.go

+		want string
+	}{
+		{
+			name: "+Addr-LocalAddr+ValidAuth",


The use of + and - to convey the presence or absence of certain fields within the Peer isn't very readable. How about the following:

withAddrAndValidAuth

withAddrLocalAddrAndValidAuth

withAddrAndEmptyAuth

withAddrLocalAddrAndEmptyAuth

nilPeer

The above names in the test output are more readable I feel.

easwars · 2024-06-26T18:54:22Z

Phew.... Didn't realize that the PR was already approved and merged.

aranjans self-requested a review April 17, 2024 17:00

aranjans suggested changes Apr 19, 2024

View reviewed changes

peer/peer.go Outdated Show resolved Hide resolved

metadata/metadata.go Outdated Show resolved Hide resolved

peer/peer_test.go Outdated Show resolved Hide resolved

aranjans assigned AnomalRoil Apr 19, 2024

AnomalRoil commented Apr 22, 2024

View reviewed changes

balancer_wrapper.go Outdated Show resolved Hide resolved

aranjans requested a review from arvindbr8 April 23, 2024 09:20

AnomalRoil added 5 commits April 24, 2024 12:04

Apply code review comment

20ee25c

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

Improve Stringer interface on acBalancerWrapper to allow human identi…

172603c

…fication of which SubConn is used Protect access to curAddr with ac.mutex Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

Use a strings.Builder in Peer.String with explicit nil fields

edfd323

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

Revert "Improve Stringer interface on acBalancerWrapper to allow huma…

985347e

…n identification of which SubConn is used" This reverts commit 001f394. Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

aranjans assigned arvindbr8 Apr 24, 2024

arvindbr8 requested changes Apr 24, 2024

View reviewed changes

arvindbr8 assigned aranjans and unassigned arvindbr8 Apr 24, 2024

arvindbr8 added the Status: Requires Reporter Clarification label Apr 24, 2024

aranjans removed their assignment Apr 25, 2024

aranjans added this to the 1.64 Release milestone Apr 29, 2024

aranjans added the Type: Behavior Change Behavior changes not categorized as bugs label Apr 29, 2024

AnomalRoil requested a review from arvindbr8 May 1, 2024 16:54

AnomalRoil removed their assignment May 1, 2024

Addressing code review comments

067b07e

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

arvindbr8 requested changes May 1, 2024

View reviewed changes

AnomalRoil commented May 2, 2024

View reviewed changes

metadata/metadata_test.go Outdated Show resolved Hide resolved

AnomalRoil and others added 2 commits May 2, 2024 12:36

Apply suggestions from code review

e482bcc

Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>

Not doing Peer's security level test

c908bad

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

aranjans removed the Status: Requires Reporter Clarification label May 2, 2024

aranjans assigned arvindbr8 May 2, 2024

Applying the rest of the code review comments

d36bfd5

Signed-off-by: Yolan Romailler <anomalroil@users.noreply.github.com>

AnomalRoil requested a review from arvindbr8 May 2, 2024 11:22

arvindbr8 reviewed May 2, 2024

View reviewed changes

arvindbr8 approved these changes May 2, 2024

View reviewed changes

arvindbr8 removed their assignment May 2, 2024

arvindbr8 requested a review from zasweq May 6, 2024 17:13

arvindbr8 assigned zasweq May 6, 2024

zasweq approved these changes May 6, 2024

View reviewed changes

zasweq changed the title ~~Satisfy the Stringer interface for Peer and Metadata~~ peer/ and metadata/:Implement the Stringer interface for Peer and Metadata May 6, 2024

zasweq changed the title ~~peer/ and metadata/:Implement the Stringer interface for Peer and Metadata~~ peer and metadata:Implement the Stringer interface for Peer and Metadata May 6, 2024

zasweq changed the title ~~peer and metadata:Implement the Stringer interface for Peer and Metadata~~ peer and metadata: Implement the Stringer interface for Peer and Metadata May 6, 2024

zasweq merged commit 9d9a96f into grpc:master May 6, 2024
12 checks passed

serathius mentioned this pull request Jun 26, 2024

[Flaking Test] UT apiserver/pkg/storage: cacher for 3m timeout kubernetes/kubernetes#125688

Closed

easwars reviewed Jun 26, 2024

View reviewed changes

dims mentioned this pull request Jun 26, 2024

metadata: make Stringer implementation consistent #7355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

AnomalRoil commented Apr 17, 2024 •

edited by zasweq

Loading

linux-foundation-easycla bot commented Apr 17, 2024 •

edited

Loading

arvindbr8 left a comment

arvindbr8 left a comment

arvindbr8 left a comment

arvindbr8 May 2, 2024

AnomalRoil May 3, 2024

easwars Jun 26, 2024

arvindbr8 commented May 2, 2024

zasweq left a comment

serathius commented Jun 26, 2024 •

edited

Loading

dims commented Jun 26, 2024

serathius commented Jun 26, 2024 •

edited

Loading

easwars Jun 26, 2024

easwars Jun 26, 2024

easwars Jun 26, 2024

easwars commented Jun 26, 2024

peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

Conversation

AnomalRoil commented Apr 17, 2024 • edited by zasweq Loading

linux-foundation-easycla bot commented Apr 17, 2024 • edited Loading

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 May 2, 2024

Choose a reason for hiding this comment

AnomalRoil May 3, 2024

Choose a reason for hiding this comment

easwars Jun 26, 2024

Choose a reason for hiding this comment

arvindbr8 commented May 2, 2024

zasweq left a comment

Choose a reason for hiding this comment

serathius commented Jun 26, 2024 • edited Loading

dims commented Jun 26, 2024

serathius commented Jun 26, 2024 • edited Loading

easwars Jun 26, 2024

Choose a reason for hiding this comment

easwars Jun 26, 2024

Choose a reason for hiding this comment

easwars Jun 26, 2024

Choose a reason for hiding this comment

easwars commented Jun 26, 2024

AnomalRoil commented Apr 17, 2024 •

edited by zasweq

Loading

linux-foundation-easycla bot commented Apr 17, 2024 •

edited

Loading

serathius commented Jun 26, 2024 •

edited

Loading

serathius commented Jun 26, 2024 •

edited

Loading