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

Add load_assignment field in Cluster #3504

Merged
merged 4 commits into from
Jun 6, 2018
Merged

Add load_assignment field in Cluster #3504

merged 4 commits into from
Jun 6, 2018

Conversation

dio
Copy link
Member

@dio dio commented May 30, 2018

Add load_assignment field in Cluster

This patch introduces load_assigment field in CDS' Cluster. This is an API change only.
This is part of effort on breaking #3261 into multiple PRs.

Risk Level:

  • Low, since it is hidden.

Testing:

  • Build api and envoy-static without error

Docs Changes:

  • Add load_assignment in Cluster of cds.proto.

Signed-off-by: Dhi Aurrahman [email protected]

// Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
//
// [#not-implemented-hide:]
ClusterLoadAssignment load_assignment = 33;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@zuercher thanks, will update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 0001298.

zuercher
zuercher previously approved these changes Jun 5, 2018
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

LGTM. I added #3553 to track the other part of the comments re: custom resolvers.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, small comment nit.

//
// Setting this allows CDS static/DNS assignments to contain embedded EDS equivalent
// :ref:`endpoint assignments<envoy_api_msg_ClusterLoadAssignment>`.
// Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
Copy link
Member

Choose a reason for hiding this comment

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

@dio can you make a note to deprecate the hosts field and add to deprecated.md when you do the PR implementing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, 2ab4f13. Thanks for reminding me.

//
// .. attention::
//
// Setting this allows CDS static/DNS assignments to contain embedded EDS equivalent
Copy link
Member

Choose a reason for hiding this comment

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

"CDS static/DNS" is a bit confusing to read. Can you rephrase? Do you mean "Setting this allows non-EDS cluster types to contain..." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated f5783e9

Signed-off-by: Dhi Aurrahman <[email protected]>
@htuch htuch merged commit 79bce5f into envoyproxy:master Jun 6, 2018
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.

4 participants