Automatically initialize AbstractEndpointSelector
#6535
Conversation
Motivation: The `initialize()` method of `AbstractEndpointSelector` may access member fields of subclasses, `initialize()` couldn't be called in the constructor of `AbstractEndpointSelector`. Although the requirement is stated in the Javadoc, many users overlooked calling `initialize()` in their implementations. I propose adding a fallback logic that automatically initializes `AbstractEndpointSelector` if it hasn't been intialized by the time the first enpoint selection occurs, so users are protected from implmentation mistakes. Modifications: - Try to initialize `AbstractEndpointSelector` if it hasn't been initialized when `select(...)` method is called. Result: Make `AbstractEndpointSelector` auto-initialize to avoid user-side implementation errors.
WalkthroughRefactors EndpointSelector initialization from eager (constructor) to thread-safe lazy initialization using an AtomicIntegerFieldUpdater; adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java (1)
254-262: Consider changing visibility toprotected.The
doSelectNowmethod inAbstractEndpointSelectoris declared asprotected abstract. While Java allows widening visibility when overriding, keeping itprotectedaligns with the design intent wheredoSelectNowis an internal hook called by the publicselectNow()method.@Override @Nullable - public Endpoint doSelectNow(ClientRequestContext ctx) { + protected Endpoint doSelectNow(ClientRequestContext ctx) { final XdsLoadBalancer loadBalancer = XdsEndpointGroup.this.loadBalancer; if (loadBalancer == null) { return null; } return loadBalancer.selectNow(ctx); }core/src/test/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelectorTest.java (1)
136-146: Consider usingprotectedvisibility fordoSelectNow.The parent class declares
doSelectNowasprotected abstract. For consistency, the override should also beprotected.private static AbstractEndpointSelector newSelector(EndpointGroup endpointGroup) { return new AbstractEndpointSelector(endpointGroup) { @Nullable @Override - public Endpoint doSelectNow(ClientRequestContext ctx) { + protected Endpoint doSelectNow(ClientRequestContext ctx) { final List<Endpoint> endpoints = endpointGroup.endpoints(); return endpoints.isEmpty() ? null : endpoints.get(0); } }; }core/src/main/java/com/linecorp/armeria/client/endpoint/DefaultEndpointSelector.java (1)
70-78: Consider changing visibility toprotected.The
doSelectNowmethod inAbstractEndpointSelectoris declared asprotected abstract. For consistency and to reflect that this is an internal implementation hook, the override should useprotectedvisibility.@Nullable @Override - public Endpoint doSelectNow(ClientRequestContext ctx) { + protected Endpoint doSelectNow(ClientRequestContext ctx) { final T loadBalancer = this.loadBalancer; if (loadBalancer == null) { return null; } return loadBalancer.pick(ctx); }core/src/test/java/com/linecorp/armeria/client/ClientRequestContextDelayedInitTest.java (1)
94-102: Consider changing visibility toprotected.For consistency with the
protected abstractdeclaration inAbstractEndpointSelector, the override should useprotectedvisibility.@Nullable @Override - public Endpoint doSelectNow(ClientRequestContext ctx) { + protected Endpoint doSelectNow(ClientRequestContext ctx) { if (counter.getAndIncrement() >= failAfter) { throw cause; } return null; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java(3 hunks)core/src/main/java/com/linecorp/armeria/client/endpoint/DefaultEndpointSelector.java(1 hunks)core/src/test/java/com/linecorp/armeria/client/ClientRequestContextDelayedInitTest.java(1 hunks)core/src/test/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelectorTest.java(3 hunks)xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/client/endpoint/DefaultEndpointSelector.javacore/src/test/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelectorTest.javacore/src/test/java/com/linecorp/armeria/client/ClientRequestContextDelayedInitTest.javacore/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointGroup.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: lint
- GitHub Check: site
- GitHub Check: flaky-tests
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (6)
core/src/test/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelectorTest.java (2)
49-55: Good test coverage for lazy initialization.The assertions properly verify that the selector is not initialized before the first selection and becomes initialized afterward. This validates the core lazy initialization behavior.
62-73: Correct verification of initialization timing for delayed selection.The test ensures that initialization occurs during
select()even when no endpoint is immediately available, which is the expected behavior.core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java (4)
42-48: Thread-safe lazy initialization implementation looks correct.Using
AtomicIntegerFieldUpdaterwith a volatile field and CAS ensures safe one-time initialization across concurrent threads without the overhead of synchronization.
76-84: CAS-based initialization is well-implemented.The early return check on line 77-79 provides a fast path for already-initialized selectors, while the CAS on line 81 ensures thread-safe one-time registration of the listener.
98-108: Good use of Template Method pattern.Making
selectNow()final and delegating todoSelectNow()ensures that initialization is guaranteed before any endpoint selection, preventing the initialization omission issue described in the PR objectives.
142-146: Unable to verify the delegation chain due to repository access limitations.The review comment makes specific technical claims about the initialization flow in
AbstractEndpointSelector.java(lines 142-146), including thattryInitialize()is idempotent with an early return and that redundant calls are safe. The original shell script request aimed to verify that no code paths bypass initialization. However, the repository could not be accessed in the sandbox environment to confirm these assertions. Manual verification against the actual codebase is required to validate the delegation chain, the idempotency oftryInitialize(), and the safety of the redundant call pattern described.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6535 +/- ##
============================================
- Coverage 74.46% 74.18% -0.29%
- Complexity 22234 23406 +1172
============================================
Files 1963 2102 +139
Lines 82437 87598 +5161
Branches 10764 11501 +737
============================================
+ Hits 61385 64981 +3596
- Misses 15918 17126 +1208
- Partials 5134 5491 +357 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @Override | ||
| public CompletableFuture<Endpoint> select(ClientRequestContext ctx, ScheduledExecutorService executor, | ||
| long timeoutMillis) { | ||
| tryInitialize(); |
There was a problem hiding this comment.
Question) For my understanding, it seems reasonable to assume that pretty much all cases of extending AbstractEndpointSelector will want to listen to the endpointGroup.
Why isn't tryInitialize() called from the constructor? Was there a race condition?
There was a problem hiding this comment.
Good question. tryInitialize() may invoke updateNewEndpoints synchronously if the endpointGroup has been updated with the initial endpoints. Since updateNewEndpoints can access member fields of subclasses, calling tryInitialize() in the constructor of AbstractEndpointSelector may result in updateNewEndpoints being invoked before those fields are initialized.
2 participants
Motivation:
As the
initialize()method ofAbstractEndpointSelectormay access member fields of subclasses,initialize()couldn't be called in the constructor ofAbstractEndpointSelector.Although the requirement is stated in the Javadoc, many users overlooked calling
initialize()in their implementations.I propose adding a fallback logic that automatically initializes
AbstractEndpointSelectorif it hasn't been initialized by the time the first endpoint selection occurs, so users are protected from implementation mistakes.Modifications:
AbstractEndpointSelectorif it hasn't been initialized whenselect(...)method is called.AbstractEndpointSelectorshould implementdoSelectNow(ctx)instead ofselectNow(ctx), which is now a final method.Result:
Make
AbstractEndpointSelectorauto-initialize to avoid user-side implementation errors.