upstream: remove cluster.hosts#6419
upstream: remove cluster.hosts#6419dio wants to merge 1 commit intoenvoyproxy:masterfrom dio:remove-hosts-1
Conversation
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
TBH, this deprecation scares me, as I think this is very widely used. It would be ideal to turn this one down for sure though. @envoyproxy/maintainers do you have thoughts on whether we should do something special here in terms of community communication or an extended deprecation cycle?
| // .. attention:: | ||
| // .. note:: | ||
| // | ||
| // Setting this allows non-EDS cluster types to contain embedded EDS equivalent |
There was a problem hiding this comment.
With hosts being done this is the only way to specify hosts right? Maybe this should be reworded a bit to make it clearer that this is not the way of specifying hosts?
|
|
||
| const std::string string_type = json_cluster.getString("type"); | ||
| auto set_dns_hosts = [&json_cluster, &cluster] { | ||
| auto set_dns_hosts = [&json_cluster, &cluster, name] { |
| const std::vector<Json::ObjectSharedPtr>& json_cluster_hosts, | ||
| bool url, bool resolved, | ||
| envoy::api::v2::ClusterLoadAssignment& load_assignment) { | ||
| // TODO(dio): Early return when json_cluster_hosts is empty. |
There was a problem hiding this comment.
Sounds like easy enough to do in this PR?
|
@htuch Agree that this is a high risk deprecation. Maybe this would warrant an envoy-announce email? |
Maybe we should do a runtime deprecation here per @alyssawilk's new stuff? Maybe she can guide us on Monday? |
|
This predates the new style deprecation but if we think it's high risk there's no harm in extending it. This is the sort of deprecation where I think our new fatal-by-default will really help make removals smoother. That said I do expect a fair amount of issues when we do our first fatal-by-default flip late this week / early next week :-P |
|
Yeah, I think running this through the new-style deprecation would make sense, +1. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
I think if we go the runtime approach, we're going to need to still keep the old code paths. I really want to see this cleanup land, but I don't think we can do it without at least going two phase due to the high risk of breaking folks.
| repeated core.Address hosts = 7 [deprecated = true]; | ||
| reserved 7; // hosts is deprecated by :ref:`load_assignment | ||
| // <envoy_api_field_Cluster.load_assignment>` | ||
| reserved "hosts"; |
There was a problem hiding this comment.
Is this the way to reserve field names to avoid future YAML/JSON clash? Should we update https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md to enforce this?
| routing to certain hosts only when there are insufficient healthy hosts available. | ||
| * upstream: add cluster factory to allow creating and registering :ref:`custom cluster type<arch_overview_service_discovery_types_custom>`. | ||
| * upstream: added a :ref:`circuit breaker <arch_overview_circuit_break_cluster_maximum_connection_pools>` to limit the number of concurrent connection pools in use. | ||
| * upstream: removed cluster.hosts. |
There was a problem hiding this comment.
removed deprecated clusters.hosts field in favor of :ref: link to the new field.
|
Sorry for not having a lot of context on this one. Since there's an existing translation from hosts to load_assignment, what's the advantage from the user's point of view in deprecating hosts field? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
6 participants
Description:
Since Cluster's load_assignment field is implemented (#3864), remove Cluster's hosts.
Risk Level: Medium
Testing: Existing tests
Release Notes: removed cluster.hosts
Fixes #4618
Signed-off-by: Dhi Aurrahman dio@tetrate.io