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.

Sign up for GitHub

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

Jump to bottom

balancer: set RPC metadata in address attributes, instead of Metadata field #4041

Merged
merged 3 commits into from
Nov 19, 2020
Merged

balancer: set RPC metadata in address attributes, instead of Metadata field #4041

merged 3 commits into from
Nov 19, 2020

Conversation

menghanl
Copy link
Contributor

This metadata will be sent with all RPCs on the created SubConn

@menghanl menghanl added the Type: Feature New features or improvements in behavior label Nov 17, 2020
@menghanl menghanl added this to the 1.34 Release milestone Nov 17, 2020
@menghanl menghanl requested a review from dfawley November 17, 2020 23:04
@menghanl
[rpc_md_in_address] balancer: set RPC metadata in address attributes,…
3330db0 
… instead of Metadata field

This metadata will be sent with all RPCs on the created SubConn
@menghanl menghanl modified the milestones: 1.34 Release, 1.35 Release Nov 17, 2020
dfawley
dfawley reviewed Nov 17, 2020
View reviewed changes
balancer/grpclb/grpclb_remote_balancer.go Outdated
@@ -164,7 +162,7 @@ func (lb *lbBalancer) refreshSubConns(backendAddrs []resolver.Address, fallback
// Create new SubConns.
for _, addr := range backendAddrs {
addrWithoutMD := addr
Copy link
Member

Choose a reason for hiding this comment

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

s/MD/Attributes/ (or Attrs)?

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

balancer/grpclb/grpclb_util.go Outdated
@@ -125,7 +125,7 @@ func (ccc *lbCacheClientConn) NewSubConn(addrs []resolver.Address, opts balancer
return nil, fmt.Errorf("grpclb calling NewSubConn with addrs of length %v", len(addrs))
}
addrWithoutMD := addrs[0]
Copy link
Member

Choose a reason for hiding this comment

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

Same

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

internal/metadata/metadata.go Outdated
Comment on lines 41 to 44
if !ok {
return nil
}
return md
Copy link
Member

Choose a reason for hiding this comment

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

Just return md, ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need ok. Just return md should work.

internal/metadata/metadata.go Outdated
Comment on lines 52 to 55
if addr.Attributes == nil {
addr.Attributes = attributes.New(mdKey, md)
return addr
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be deleted, as WithValues handles a nil receiver.

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

test/balancer_test.go Outdated
Comment on lines 582 to 583
r := manual.NewBuilderWithScheme("whatever")
t.Logf("Registered manual resolver with scheme %s...", r.Scheme())
Copy link
Member

Choose a reason for hiding this comment

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

Is this registering? It doesn't need to (WithResolvers) right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not registered. So we will need WithResolvers. Removed this line.

test/balancer_test.go Outdated
Comment on lines 590 to 593
select {
case <-testMDChan:
default:
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Made it a switch with context.Done().

@dfawley dfawley assigned menghanl and unassigned dfawley Nov 17, 2020
dfawley
dfawley reviewed Nov 18, 2020
View reviewed changes
test/balancer_test.go Outdated
@@ -601,6 +598,7 @@ func (s) TestMetadataInAddressAttributes(t *testing.T) {
}
defer ss.Stop()

r := manual.NewBuilderWithScheme("whatever")
Copy link
Member

Choose a reason for hiding this comment

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

Did you know? You can inject this into the stubserver and Dial will use it:

grpc-go/test/end2end_test.go

Lines 5197 to 5200 in 230166b

if ss.r != nil {
ss.target = ss.r.Scheme() + ":///" + ss.address
opts = append(opts, grpc.WithResolvers(ss.r))
}

Maybe we should delete that feature since it seems to be well hidden and nonobvious? Or you should use it here.

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 knew it. But you cannot set balancer/init service config.
I didn't want to add all the fields just to pass the dial options.

I'm OK with removing it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing where your test needs to set balancer/init service config, though. You are using a default service config, and then doing the same resolver update that stubServer would do automatically.

Copy link
Contributor Author

@menghanl menghanl Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm dumb. I need to set the balancer in init service config, to use the balancer that sets the attributes. But ss.Start works. Fixed now. (But github hates me?)

@menghanl menghanl merged commit 7886479 into grpc:master Nov 19, 2020
@menghanl menghanl deleted the rpc_md_in_address branch November 19, 2020 01:12
davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020
@menghanl @davidkhala
balancer: set RPC metadata in address attributes, instead of Metadata…
9aa05f3 
… field (grpc#4041)

This metadata will be sent with all RPCs on the created SubConn
This was referenced Mar 8, 2021
Open
Open
Closed
This was referenced Mar 16, 2021
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@menghanl menghanl

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.35 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@menghanl @dfawley