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

xds: support pick_first custom load balancing policy (A62) #6314

Merged
merged 7 commits into from May 30, 2023

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented May 24, 2023

RELEASE NOTES:

  • xds: support pick_first custom load balancing policy (A62)

@dfawley dfawley added this to the 1.56 Release milestone May 24, 2023
@dfawley dfawley requested a review from zasweq May 24, 2023 18:49
@zasweq
Copy link
Contributor

zasweq commented May 24, 2023

Will take a look, but fyi this is failing vet and tests :).

@dfawley
Copy link
Contributor Author

dfawley commented May 24, 2023

Will take a look, but fyi this is failing vet and tests :).

Forgot to remove a line from my testing...should be fixed now.

@@ -382,6 +383,8 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {
}

func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Top level comment about setup and expectation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not be detailed and explain the whole test setup and everything in the comment. The name is very self-explanatory, but I added a short comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I mean. I just feel like it should be briefly explain stuff like configures pick_first as top level lb of the channel, sends it address list before knob and connection isn't random, after knob it is random (with our determinisitic randomness algorithm plumbed). See

// TestPickFirst_NewAddressWhileBlocking tests the case where pick_first is
for an example of what I'm going for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is unnecessarily detailed and a recipe for the test & comment diverging when someone decides to change the test to add an extra check or do things in a different order because of a race, etc. I'd rather not over-document our tests, and would definitely not require detailed documentation anywhere. It's actually pretty rare to have any comments at the top of a test.

E.g. https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/net/http/client_test.go;l=550

@@ -431,3 +434,58 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
t.Fatal(err)
}
}

func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Please add top level comment with setup and expectation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

pickfirst.go Show resolved Hide resolved
test/pickfirst_test.go Show resolved Hide resolved
},
},
wantConfig: `[{"pick_first": { "shuffleAddressList": true }}]`,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: can you add a test case for it being unset and it creating false? I guess that is specific and might be more appropriate (unset and also set to false -> false as per my comment on the main code) in ParseConfig test in pick_first rather than scope it to this test here which tests the registry. Actually since the registry does incorporate this proto -> json conversion, maybe we should add tests here. Or maybe that would scale up the codebase too much but I'm now mindful of those 3 buckets (not set, set and zero, set and non zero) in both the xDS flow through proto and the user writing json itself in the file system e.g. and how those 3 buckets map to our balancer's eventual internal configuration type returned from ParseConfig().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no "unset" in the PF proto input. This is a boolean field. If it's missing from the struct, it defaults to its zero value of false. That said, testing that it outputs false when the input is false/absent-so-zero is worthwhile, so done.

Copy link
Contributor

@zasweq zasweq May 26, 2023

Choose a reason for hiding this comment

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

Oh. I've seen the proto bools be wrapped in a proto pointer type. Right, now if it's not explicitly set it gets it's zero (false, not nil because it's a bool value type), and it should make a false config in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen the proto bools be wrapped in a proto pointer type.

There are wrapper types for use cases where presence is important. That's not in use here, because presence is not important for this field (missing=default=zero=false).

test/pickfirst_test.go Show resolved Hide resolved
test/pickfirst_test.go Show resolved Hide resolved
test/pickfirst_test.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq May 25, 2023
@zasweq
Copy link
Contributor

zasweq commented May 25, 2023

Btw there's a ton of merge conflicts with go.mod/go.sum stuff.

Copy link
Contributor Author

@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; comments addressed

test/pickfirst_test.go Show resolved Hide resolved
test/pickfirst_test.go Show resolved Hide resolved
test/pickfirst_test.go Show resolved Hide resolved
@@ -382,6 +383,8 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {
}

func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not be detailed and explain the whole test setup and everything in the comment. The name is very self-explanatory, but I added a short comment.

@@ -431,3 +434,58 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
t.Fatal(err)
}
}

func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go Outdated Show resolved Hide resolved
pickfirst.go Show resolved Hide resolved
@@ -198,10 +223,7 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
},
},
},
wantConfig: &internalserviceconfig.BalancerConfig{
Name: "myorg.MyCustomLeastRequestPolicy",
Config: customLBConfig{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you're trying to say.

We need to register the LB policy in order for the code to work, right? The custom LB policy would not be emitted if it's not registered.

t.Fatalf("ConvertToServiceConfig() got unexpected output, diff (-got +want): %v", diff)
}
})
}
}

func jsonMarshal(t *testing.T, x interface{}) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

x LGTM for a 5 line function.

@dfawley dfawley assigned zasweq and unassigned dfawley May 25, 2023
@dfawley
Copy link
Contributor Author

dfawley commented May 25, 2023

I'll fix the go.mod/sum stuff before merging

@@ -301,6 +314,8 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
if err != nil {
t.Fatalf("ConvertToServiceConfig(%s) failed: %v", pretty.ToJSON(test.policy), err)
}
// got and want must be unmarshalled since JSON strings shouldn't
// generally be directly compared.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: briefly mention why. E.g. removes whitespaces/nondeterministic creation/functionality equiavlent JSON even with whitespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can assume a programmer understands why JSON strings can't be directly compared.

@@ -112,7 +113,7 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState
b.cfg = cfg
}

if b.cfg != nil && b.cfg.ShuffleAddressList {
if envconfig.PickFirstLBConfig && b.cfg != nil && b.cfg.ShuffleAddressList {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does every language guarantee this optimization that we as a team (I use it a lot) to protect panics if x == nil || x.y (will panic but first check not exiting guarantees this won't panic). Hopefully Go never changes this early evaluation of the conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, short-circuiting is fundamental to every major programming language.

t.Fatalf("Error unmarshalling rawJSON (%q): %v", rawJSON, err)
}
var want []map[string]interface{}
if err := json.Unmarshal(json.RawMessage(test.wantConfig), &want); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure this is WAI by deleting a recursion layer of wrrLocalityBalancerConfig and making sure this fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you're concerned about specifically, but I did this and it failed.

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM outside of a few trailing comments/nits.

@zasweq zasweq assigned dfawley and unassigned zasweq May 26, 2023
@dfawley dfawley merged commit 6c2529b into grpc:master May 30, 2023
11 checks passed
@dfawley dfawley deleted the pf_xds branch May 30, 2023 16:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2023
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