Modify behavior of Endpoint#withAttrs to merge attributes
#5802
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5802 +/- ##
============================================
+ Coverage 74.75% 74.76% +0.01%
- Complexity 21409 21421 +12
============================================
Files 1877 1877
Lines 79126 79170 +44
Branches 10201 10208 +7
============================================
+ Hits 59152 59195 +43
- Misses 15179 15182 +3
+ Partials 4795 4793 -2 ☔ View full report in Codecov by Sentry. |
minwoox
approved these changes
Jul 8, 2024
| * in {@param newAttributes} has higher precedence. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public Endpoint withAttrs(Attributes newAttributes) { |
jrhee17
added a commit
that referenced
this pull request
Aug 8, 2024
Motivation: It is recommended to review #5802 prior to this PR This changeset attempts to solve several problems: ##### Custom filter logic xDS considers all endpoints when computing whether a `PrioritySet` is in panic state. For instance, if the percentage of unhealthy endpoints exceeds a preconfigured panic threshold, the endpoint selection includes all endpoints regardless of the degraded status. While armeria supports an `HealthCheckedEndpointGroup` out of the box, it filters out healthy endpoints automatically. - https://github.com/line/armeria/blob/72ebfe12abf7804723a490fe7acfe3a45d46089f/xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLoadBalancer.java#L99-L105 - ref: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/panic_threshold In order to resolve this, I propose that a `AbstractHealthCheckedEndpointGroupBuilder#healthCheckedEndpointPredicate` API is added ##### Per-cluster health check configuration Per-cluster member health check is difficult with the current API since a single parameter set is statically defined for an entire health checked endpoint group. - ref: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/health_checking#per-cluster-member-health-check-config We already have an abstraction `AbstractHealthCheckedEndpointGroupBuilder#newCheckerFactory`. I propose that this API be used for the purpose of xDS. In order to support the parameters xDS allows configuring, I propose that parameters are passed to `HttpHealthChecker` via the constructor instead of the `HealthCheckerContext`. In order to support this change, `HttpHealthChecker` has been moved to an internal package so the `xds` module can also access it. Modifications: - Added APIs `AbstractHealthCheckedEndpointGroupBuilder#healthCheckedEndpointPredicate` and modified `HealthCheckedEndpointGroup` to filter endpoints based on the predicate. - Health checked `Endpoint`s now have attributes `HEALTHY` and `DEGRADED` set. - `HealthCheckedEndpointGroup#setEndpoints` is now called on every invocation of `updateHealth`, not just when a health status changes. - xDS sometimes needs to override the health checked `Endpoint`, so modified to receive health checked parameters from the `HttpHealthChecker` constructor instead of `HealthCheckerContext`. - In the process, `HttpHealthChecker` has been moved to an internal package - `HealthCheckerContext#originalEndpoint` has been added to retrieve the original endpoint. The other APIs haven't been deprecated since it is potentially useful information that other `HealthChecker`s may be using. - Introduced a `XdsHealthCheckedEndpointGroupBuilder` which implements its own `checkerFactory`. - `EndpointUtil#coarseHealth` has been updated to consider `HEALTHY`, `DEGRADED` attributes when determining health. Result: - The behavior of `XdsEndpointGroup` is more aligned with envoy in terms of health checking - Preparation for zone-aware load balancing <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
It seems inconsistent that
Endpoint#withAttrmerges the attribute to the existing attributes, whereasEndpoint#withAttrssimply replaces the previous attributes.I propose that we modify the behavior of
Endpoint#withAttrsto be aligned withEndpoint#withAttr. Additionally, in order to support the previous behavior, I propose a new APIEndpoint#replaceAttrsbe added.I've confirmed that there are no uses of this API at least internally.
ref: #5785 (comment)
Modifications:
Endpoint#withAttrsto merge attributesEndpoint#replaceAttrsto support the previous behaviorResult: