Skip to content
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.

Sign up for GitHub

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

Jump to bottom

loadbalancer-experimental: consolidate outlier detector concerns into the OutlierDetectorConfig #2864

Merged
merged 3 commits into from
Mar 26, 2024
Merged

loadbalancer-experimental: consolidate outlier detector concerns into the OutlierDetectorConfig #2864

merged 3 commits into from
Mar 26, 2024

Conversation

bryce-anderson
Copy link
Contributor

@bryce-anderson bryce-anderson commented Mar 7, 2024 •
edited
Loading

Motivation:

We currently have two different outlier detector implementations: the xDS compatible implementation and the L4 connection failure implementation. Right now the configuration of them is odd: one is configured on LoadBalancerBuilder and the other is passed to the xds outlier detector factory. This is a really strange API.

Modifications:

  • move l4 configuration options to OutlierDetectorConfig so everything is in one place
  • remove the LoadBalancerBuilder.outlierDetector(..) method in favor of one taking the OutlierDetectorConfig
  • make service-discovery re-subscribing enabled even if consecutive connect failures is not

@bryce-anderson bryce-anderson force-pushed the bl_anderson/DefaultLoadBalancerBuilderCleanup branch from c393042 to 7a1b5fe Compare March 14, 2024 18:32
@bryce-anderson
loadbalancer-experimental: consolidate outlier detector concerns into…
0b9b892 
… the OutlierDetectorConfig

Motivation:

We currently have two different outlier detector implementations:
the xDS compatible implementation and the L4 connection failure
implementation. Right now the configuration of them is odd: one is
configured on LoadBalancerBuilder and the other is passed to the
xDS outlier detector factory. This is a really strange API.

Modifications:

- move l4 configuration options to OutlierDetectorConfig so
  everything is in one place
- remove the LoadBalancerBuilder.outlierDetector(..) method in
  favor of one taking the OutlierDetectorConfig
- make service-discovery re-subscribing enabled even if consecutive
  connect failures is not
@bryce-anderson bryce-anderson force-pushed the bl_anderson/DefaultLoadBalancerBuilderCleanup branch from db8f77c to 0b9b892 Compare March 15, 2024 20:13
@bryce-anderson bryce-anderson marked this pull request as ready for review March 15, 2024 20:13
bryce-anderson
bryce-anderson commented Mar 15, 2024
View reviewed changes
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java
@@ -30,7 +41,19 @@
*/
public final class OutlierDetectorConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the most important change in this PR, specifically it is now the core API used to configure all types of outlier detection.

bryce-anderson
bryce-anderson commented Mar 15, 2024
View reviewed changes
...loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/LoadBalancerBuilder.java
* @return {@code this}.
* @see #healthCheckFailedConnectionsThreshold(int)
*/
LoadBalancerBuilder<ResolvedAddress, C> healthCheckInterval(Duration interval, Duration jitter);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of these methods is part of the biggest change in the PR: modification of the API by moving the outlier detector configuration all to OutlierDetectorConfig.

bryce-anderson
bryce-anderson commented Mar 15, 2024
View reviewed changes
...loadbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/NoopOutlierDetector.java

// A simple outlier detector implementation that only provides the basic `RequestTracker` implementation
// so the P2C LoadBalancingPolicy can still be effective.
final class NoopOutlierDetector<ResolvedAddress, C extends LoadBalancedConnection>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a premature optimization: We can probably do without this and just use the XdsOutlierDetector with everything disable if we wanted. This does make the testing easier: otherwise a few tests pop because they expect a certain number of timer tasks and the xds detector makes one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really NoOp then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean the timer task then it would be in that situation since none of the detectors will ever signal an outlier. We could special case that if we wanted as well, but at least at the time this felt better. Then it felt meh, thus the self flagging for other opinions.

tkountis
tkountis reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@tkountis tkountis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM - i like the single config point.
I dont if we discussed again, but wondering if it makes sense to support a concept of OutlierDetector[] (eg. filters) vs a single view.

@bryce-anderson
Copy link
Contributor Author

bryce-anderson commented Mar 22, 2024 •
edited
Loading

The change LGTM - i like the single config point. I dont if we discussed again, but wondering if it makes sense to support a concept of OutlierDetector[] (eg. filters) vs a single view.

@tkountis and I talked offline about this. (Thomas, please correct me if I misrepresent our conclusions).
And this time we're intentionally be opinionated about the interface. That lets the simple cases have a better user experience and makes it easier for frameworks to inspect what is going on.

The downside is that it makes this builder less flexible. However, we don't want to encourage people to make their own OutlierDetector instances, the detectors we have now should be enough for most use cases and aligns well with our longer term goals. When the time comes that someone really really wants/needs to we can either make a 'power user' API or expose the raw types for them, but overall we're less focused on make that pathway comfortable.

@bryce-anderson bryce-anderson requested a review from mgodave March 25, 2024 18:15
idelpivnitskiy
idelpivnitskiy reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New API LGTM!

...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java Outdated Show resolved Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java Outdated Show resolved Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java Outdated Show resolved Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java Outdated Show resolved Hide resolved
...adbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorConfig.java Outdated Show resolved Hide resolved
...ancer-experimental/src/main/java/io/servicetalk/loadbalancer/DefaultLoadBalancerBuilder.java Outdated Show resolved Hide resolved
...dbalancer-experimental/src/main/java/io/servicetalk/loadbalancer/OutlierDetectorFactory.java Outdated Show resolved Hide resolved
@bryce-anderson bryce-anderson merged commit 1f31e61 into apple:main Mar 26, 2024
15 checks passed
@bryce-anderson bryce-anderson deleted the bl_anderson/DefaultLoadBalancerBuilderCleanup branch March 26, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelpivnitskiy idelpivnitskiy idelpivnitskiy approved these changes

@tkountis tkountis tkountis approved these changes

@daschl daschl Awaiting requested review from daschl

@Scottmitch Scottmitch Awaiting requested review from Scottmitch

@mgodave mgodave Awaiting requested review from mgodave

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bryce-anderson @tkountis @idelpivnitskiy