-
Notifications
You must be signed in to change notification settings - Fork 918
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
Implement zone aware load balancing for XdsEndpointGroup
#5808
Conversation
caa6459
to
1a386a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 👍
In fact, I approve this PR without reviewing it in detail so that you can continue working on your tasks. I will separately check and compare the Envoy implementation and ours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very promising. 👍
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterManager.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterManager.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterManager.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterEntry.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLoadBalancer.java
Outdated
Show resolved
Hide resolved
.collect(Collectors.toList()); | ||
final ImmutableList.Builder<ResidualCapacity> residualCapacity = ImmutableList.builder(); | ||
// to guarantee that residualCapacity has at least one element | ||
residualCapacity.add(new ResidualCapacity(localLocality, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this localLocality
will never be chosen. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localLocality
could be chosen in the extreme case if all ResidualCapacity
have 0
armeria/xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/LocalityRoutingStateFactory.java
Lines 174 to 177 in 885f7f8
// This is *extremely* unlikely but possible due to rounding errors when calculating | |
// locality percentages. In this case just select random locality. | |
final int idx = random.nextInt(residualCapacities.size(), RandomHint.ALL_RESIDUAL_ZERO); | |
return residualCapacities.get(idx).locality; |
Having said this, the fact that this code is called implies that upstream already has healthy/degraded localLocality
hosts so I guess this won't be a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the code. Thanks for the explanation. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding this excellent feature.
You're the MVP. 👍 👍 👍
listenerRoot.close(); | ||
} | ||
final ImmutableList.Builder<CompletableFuture<?>> closeFuturesBuilder = ImmutableList.builder(); | ||
closeFuturesBuilder.addAll(clusterEntries.clusterEntriesMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
closeFuturesBuilder.addAll(clusterEntries.clusterEntriesMap | |
closeFuturesBuilder.addAll(clusterEntriesMap() |
Motivation:
This changeset attempts to implement envoy's zone aware routing.
A small sample on how this can be configured can be seen here as well.
The basic algorithm in layman's terms is the following:
The code has been mostly adapted from envoy's implementation:
Modifications:
ListenerRoot
when creating anXdsEndpointGroup
. However, we need more information like local locality from the bootstrap. To make this intention clear,XdsEndpointGroup
now accepts anXdsBootstrap
. Additionally,XdsBootstrap#bootstrap
has been added to access the local cluster name and locality.ClusterManager
now maintains aLocalCluster
data structure which watches changes in the local cluster if exists. When aClusterEntry
is created, the existingLocalCluster
is passed.ClusterEntry
needs to listen to the localClusterEntry
for updates in order to pre-compute the state.ClusterEntry
is now anAbstractListenable
and notifies the currentLoadBalancer
.PrioritySet
is required to compute the locality state. Added aLoadBalancer#prioritySet
method to retrieve the currentPrioritySet
.XdsLoadBalancer
has been added as an extra abstraction layer to avoid conflicts with AddLoadBalancer
for generalizingEndpointSelector
#5779ClusterEntry
now has two update points: 1) when the endpoints are updated 2) when the local cluster is updated. Now, the two entry points call a singletryRefresh
to refresh the load balancer.LocalityRoutingStateFactory
and modifiedDefaultLoadBalancer
to support zone aware routing. The implementation should look very similar to envoy's source.XdsRandom
interface for easier testing. Additionally, to guarantee that the local cluster was fully loaded before proceeding with tests,ClusterEntry#initialLocalEntryStateFuture
has been added.XdsEndpointGroup#clusterEntries
, the factory methods forXdsEndpointGroup
now return the concrete type.Result:
XdsEndpointGroup
now supports zone-aware load balancing.