Order services by name to do not generate CDS when service state in remote has chanfed #159
Conversation
| @@ -89,6 +89,7 @@ class EnvoySnapshotFactory( | |||
| } | |||
|
|
|||
| return addRemovedClusters(previousClusters, currentClusters) | |||
| .toSortedMap(Comparator.naturalOrder()) | |||
There was a problem hiding this comment.
Why we need Comparator.naturalOrder()? I think it's a default one.
There was a problem hiding this comment.
Yes, it is. I wanted to make it clear that we need naturalOrder and if a default would change we won't break.
There was a problem hiding this comment.
I think we won't break if the default changes, because we are not interested in how the entries are sorted, we only require them to be sorted consistently.
Side note: there is really no chance that the default will change. It would be a breaking change and there is really no other candidate to set as a default.
But I see no problem with stating it explicitly, so for me we can keep it.
| @@ -89,6 +89,7 @@ class EnvoySnapshotFactory( | |||
| } | |||
|
|
|||
| return addRemovedClusters(previousClusters, currentClusters) | |||
| .toSortedMap(Comparator.naturalOrder()) | |||
There was a problem hiding this comment.
This change may help remove superfluous changes but this code does not communicate intent. I see this toSortedMap and wonder why this needs to be a sorted map and why it's created in this place.
I'd also change return type of clusterConfigurations to SortedMap<String, ClusterConfiguration>.
| @@ -51,6 +51,7 @@ import java.util.concurrent.Executors | |||
| import java.util.concurrent.TimeUnit | |||
| import java.util.function.Consumer | |||
|
|
|||
| @Suppress("LargeClass") | |||
There was a problem hiding this comment.
I'd add an issue to think about extracting some parts of SnapshotUpdater and breaking things up.
| @@ -88,7 +89,10 @@ class EnvoySnapshotFactory( | |||
| .associateWith { ClusterConfiguration(serviceName = it, http2Enabled = false) } | |||
| } | |||
|
|
|||
| // Clusters need to be sorted because if clusters are in different order to previous snapshot then CDS version | |||
There was a problem hiding this comment.
It looks like something that should belong to the contract in java-control-plane's Snapshot, right?
lukidzi
dismissed stale reviews from chemicL, slonka, and MarcinFalkowski
via
b9c5768
Added ordering of global snapshot to keep CDS version same when the order of services from remote has changed.
Didn't change
List<Cluster>to Set` because we would have to do the same operation for the same group. Here we can do this just once in global snapshot.