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

grpc: do not use balancer attributes during address comparison #6439

Merged
merged 4 commits into from Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 13 additions & 12 deletions attributes/attributes.go
Expand Up @@ -112,30 +112,31 @@ func (a *Attributes) String() string {
sb.WriteString("{")
first := true
for k, v := range a.m {
var key, val string
if str, ok := k.(interface{ String() string }); ok {
key = str.String()
} else if str, ok := k.(string); ok {
key = str
}
if str, ok := v.(interface{ String() string }); ok {
val = str.String()
} else if str, ok := v.(string); ok {
val = str
}
if !first {
sb.WriteString(", ")
}
sb.WriteString(fmt.Sprintf("%q: %q ", key, val))
sb.WriteString(fmt.Sprintf("%s: %s ", str(k), str(v)))
first = false
}
sb.WriteString("}")
return sb.String()
}

func str(x interface{}) string {
if v, ok := x.(fmt.Stringer); ok {
return fmt.Sprintf("%q", v.String())
} else if v, ok := x.(string); ok {
return fmt.Sprintf("%q", v)
}
return fmt.Sprintf("<%p>", x)
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't print literal quotes.

Why not keep it the way it was? Have this return the actual string and quote it when it's used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense.

}

// MarshalJSON helps implement the json.Marshaler interface, thereby rendering
// the Attributes correctly when printing (via pretty.JSON) structs containing
// Attributes as fields.
dfawley marked this conversation as resolved.
Show resolved Hide resolved
//
// Is it impossible to unmarshal attributes from a JSON representation and this
// method is meant only for debugging purposes.
func (a *Attributes) MarshalJSON() ([]byte, error) {
return []byte(a.String()), nil
}
9 changes: 6 additions & 3 deletions internal/stubserver/stubserver.go
Expand Up @@ -56,9 +56,12 @@ type StubServer struct {

// Parameters for Listen and Dial. Defaults will be used if these are empty
// before Start.
Network string
Address string
Target string
Network string
Address string
Target string

// Custom listener to use for serving. If unspecified, a new listener is
// created on a local port.
Listener net.Listener
Copy link
Member

Choose a reason for hiding this comment

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

This is not a parameter for Listen or Dial. Can you document this please and indicate Network & Address aren't used if this is set, and that Target is the responsibility of the user to set properly when Listener is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My code search reveals that nobody is setting Network, Address or Target.

Address is used as a read-only field by numerous tests to retrieve the address of the stub server. Should we just get rid of Network and Target and make Address as a read-only field, or make it a getter method? (Can do in a follow up PR)

Copy link
Member

Choose a reason for hiding this comment

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

SGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do the cleanup in a follow up PR. Thanks.


cleanups []func() // Lambdas executed in Stop(); populated by Start().
Expand Down
8 changes: 1 addition & 7 deletions test/pickfirst_test.go
Expand Up @@ -351,13 +351,7 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {
}
t.Cleanup(func() { cc.Close() })

// Wait for the channel to move to TransientFailure.
for state := cc.GetState(); state != connectivity.TransientFailure; state = cc.GetState() {
if !cc.WaitForStateChange(ctx, state) {
t.Errorf("Timeout when waiting for state to change to TransientFailure. Current state is %s", state)
return
}
}
awaitState(ctx, t, cc, connectivity.TransientFailure)

// Spawn a goroutine to ensure that the channel stays in TransientFailure.
// The call to cc.WaitForStateChange will return false when the main
Expand Down