Skip to content

Update LB proto comment again#28

Merged
AspirinSJL merged 1 commit intogrpc:masterfrom
AspirinSJL:lb_comment
Jun 22, 2018
Merged

Update LB proto comment again#28
AspirinSJL merged 1 commit intogrpc:masterfrom
AspirinSJL:lb_comment

Conversation

@AspirinSJL
Copy link
Copy Markdown
Contributor

I changed this in grpc repo before. I feel it's worth keeping because it does clarify the port thing.

@AspirinSJL
Copy link
Copy Markdown
Contributor Author

Amended with nit.

Copy link
Copy Markdown
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

LGTM

// Name of load balanced service (IE, service.googleapis.com). Its
// length should be less than 256 bytes.
// The name of the load balanced service (e.g., balancer.service.com). The max
// length of the name is 256 bytes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

256 or 255?

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.

I just checked with the LB folks. We actually don't see apparent limitation to this length in our LB implementation. We definitely need to discuss it and make the requirement clear. Before any decision is made, let me keep the old one.

@AspirinSJL AspirinSJL merged commit 91d19ac into grpc:master Jun 22, 2018
@AspirinSJL AspirinSJL deleted the lb_comment branch June 22, 2018 20:35
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.

2 participants