Skip to content

grpc/service_config: Add service_name field to grpclb config.#73

Merged
easwars merged 3 commits intogrpc:masterfrom
easwars:grpclb_target_name
Feb 7, 2020
Merged

grpc/service_config: Add service_name field to grpclb config.#73
easwars merged 3 commits intogrpc:masterfrom
easwars:grpclb_target_name

Conversation

@easwars
Copy link
Copy Markdown
Contributor

@easwars easwars commented Feb 6, 2020

This will be used by rls to override the name of the target to be sent
to the remote balancer.

This will be used by rls to override the name of the target to be sent
to the remote balancer.
@easwars easwars requested a review from markdroth February 6, 2020 16:04
// the list in order and stop at the first policy that they support.
repeated LoadBalancingConfig child_policy = 1;
// If specified, overrides the name of the target to be sent to the balancer.
optional string target_name = 2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is proto3, so no need for "optional".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@easwars
Copy link
Copy Markdown
Contributor Author

easwars commented Feb 6, 2020

Also, realized that we don't have a BUILD rule for this proto. I'm not sure if that is by design. I made the BUILD rule changes locally to be able to build this proto. I could send a PR if it makes sense to do so.

@markdroth
Copy link
Copy Markdown
Member

Also, realized that we don't have a BUILD rule for this proto. I'm not sure if that is by design. I made the BUILD rule changes locally to be able to build this proto. I could send a PR if it makes sense to do so.

I tried to put together a PR to add the BUILD rule for this a while back (see #59), but I ran into problems and haven't found time to figure out what was wrong. If you want to pick that up and drive it forward, that would be great.

Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@ejona86
Copy link
Copy Markdown
Member

ejona86 commented Feb 6, 2020

Uhh... this is unnecessary/awkward in Java. grpclb observes the authority, which can be changed by overriding the Helper. So it doesn't seem this is necessary. Also, we don't send target strings to the balancer, right?

Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

See my other comment

Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Assuming the rename, LGTM

repeated LoadBalancingConfig child_policy = 1;
// Optional. If specified, overrides the name of the target to be sent to
// the balancer.
string target_name = 2;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@markdroth and I talked. I understand now that we need such a field, and this isn't a normal "target" string. Let's go with service_name, to try to match the nomenclature we're using in the grpclb protocol.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@easwars easwars merged commit 1ff7890 into grpc:master Feb 7, 2020
@easwars easwars deleted the grpclb_target_name branch February 7, 2020 04:25
easwars added a commit to easwars/grpc-go that referenced this pull request Feb 7, 2020
dfawley pushed a commit to grpc/grpc-go that referenced this pull request Feb 7, 2020
@markdroth markdroth changed the title grpc/service_config: Add target_name field to grpclb config. grpc/service_config: Add service_name field to grpclb config. Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants