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

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Jul 11, 2023

Summary of changes:

  • Make of copy of received addresses in the clientconn code during subchannel creation and address update operation
    • clear out the balancer attributes field in this address list, thereby ensuring that changes in balancer attributes don't lead to connection churn
  • Minor improvements to rendering of attributes in logs
  • Make it possible to pass a net.Listener to the stubserver
    • this allows tests to use a wrapped listener that notifies the test upon connection establishment

Fixes internal issue: b/290272899

RELEASE NOTES:

  • grpc: eliminate connection churn during an address update that differs only in balancer attributes

@easwars easwars requested a review from dfawley July 11, 2023 01:38
@easwars easwars added this to the 1.57 Release milestone Jul 11, 2023
Comment on lines 116 to 120
if str, ok := k.(interface{ String() string }); ok {
key = str.String()
} else if str, ok := k.(string); ok {
key = str
}
Copy link
Member

Choose a reason for hiding this comment

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

Sad that string doesn't implement Stringer 😞

To consolidate these code paths, and to avoid printing literally ", " if key & val are both not string-y, maybe we should have something like this?:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Had to add a "%q" format string to ensure that the key and value strings are quoted.

attributes/attributes.go Show resolved Hide resolved
Network string
Address string
Target string
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.

test/pickfirst_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned easwars and unassigned dfawley Jul 11, 2023
} 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.

@easwars easwars assigned dfawley and unassigned easwars Jul 11, 2023
@dfawley dfawley assigned easwars and unassigned dfawley Jul 11, 2023
@easwars easwars merged commit 8e9c8f8 into grpc:master Jul 12, 2023
11 checks passed
@easwars easwars deleted the address_update_equality_comparison branch July 12, 2023 01:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants