Skip to content

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 dio@rockybars.com

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
// 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.

I think the discussion at https://github.com/envoyproxy/envoy/pull/3261/files#r188243657 still applies.

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.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
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 <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@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