Make it possible to permit empty endpoints in CD endpoint groups - #1319
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesallowEmptyEndpoints propagation through EndpointGroup construction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java`:
- Around line 129-133: The `allowEmptyEndpoints` parameter is passed to the
parent DynamicEndpointGroup constructor but is not being honored in the refresh
logic. In the refresh method around line 143 where empty decoded updates are
dropped unconditionally, add a check for the `allowEmptyEndpoints` flag. Only
drop empty endpoint updates if `allowEmptyEndpoints` is false; when it is true,
allow the empty endpoint list to be processed and reflected in the endpoint
group. This ensures that transitions to empty endpoint states are properly
honored when explicitly enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8be4af5e-979f-4d75-91d2-56ec0362ddbb
📒 Files selected for processing (2)
client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.javaclient/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroupBuilder.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java (1)
123-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd Javadoc for the
allowEmptyEndpointsparameter.The three-parameter
builderoverload introduces anallowEmptyEndpointsparameter but does not document it. Add an@param allowEmptyEndpointstag explaining when and why a caller would set this totrue.📝 Suggested Javadoc addition
/** * Returns a new {`@link` CentralDogmaEndpointGroupBuilder} with the {`@link` Watcher} * and {`@link` EndpointListDecoder}. You can create a {`@link` Watcher} using {`@link` CentralDogma}: * * <pre>{`@code` * CentralDogma centralDogma = ... * Query<T> query = ... // The query to the entry that contains the list of endpoints. * Watcher watcher = centralDogma.fileWatcher(projectName, repositoryName, query); * }</pre> + * + * `@param` allowEmptyEndpoints whether to allow the endpoint group to have an empty endpoint list. + * When {`@code` false}, transitions to an empty endpoint list are ignored + * and the group retains its previous endpoints. When {`@code` true}, the + * group will reflect an empty state when the watched entry contains no endpoints. */ public static <T> CentralDogmaEndpointGroupBuilder<T> builder(Watcher<T> watcher,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java` around lines 123 - 127, The static builder method in the CentralDogmaEndpointGroupBuilder class is missing Javadoc documentation for the allowEmptyEndpoints parameter. Add a Javadoc comment block above the builder method that includes an `@param` tag for the allowEmptyEndpoints parameter, explaining what this parameter controls and the conditions or reasons when a caller would set it to true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java`:
- Around line 123-127: The static builder method in the
CentralDogmaEndpointGroupBuilder class is missing Javadoc documentation for the
allowEmptyEndpoints parameter. Add a Javadoc comment block above the builder
method that includes an `@param` tag for the allowEmptyEndpoints parameter,
explaining what this parameter controls and the conditions or reasons when a
caller would set it to true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac8753dc-5634-43b0-a8ba-db58ac7615b8
📒 Files selected for processing (1)
client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java
|
Build error looks like an unrelated flake? |
| */ | ||
| public static <T> CentralDogmaEndpointGroupBuilder<T> builder(Watcher<T> watcher, | ||
| EndpointListDecoder<T> endpointListDecoder, | ||
| boolean allowEmptyEndpoints) { |
There was a problem hiding this comment.
Optional) Given that we're already using a builder pattern and allowEmptyEndpoints's default value is reasonable, I wonder if it's a better idea to add a builder method instead of adding to the builder factory method.
There was a problem hiding this comment.
The use sites I've seen don't really use it as a builder, they all call the factory method and build() immediately, so this seemed more in line with existing use? But happy to follow the builder style if you prefer.
There was a problem hiding this comment.
My line of thought is just from an API management point - each param could possibly add 2^n overload variants.
There was a problem hiding this comment.
Makes sense. Doing it that way.
| try { | ||
| final List<Endpoint> newEndpoints = endpointListDecoder.decode(instances); | ||
| if (newEndpoints.isEmpty()) { | ||
| if (newEndpoints.isEmpty() && !allowsEmptyEndpoints()) { |
There was a problem hiding this comment.
I understand this is the main change because empty endpoints are allowed in DynamicEndpointGroup by default.
There was a problem hiding this comment.
Hmm I guess so. Didn't dig in far enough, just saw it would exception when trying to start if there were no endpoints.
There was a problem hiding this comment.
Checked the consequences of setting to false rather than true, seems to just be early exit from update methods that would otherwise set it to empty.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroupBuilder.java (1)
67-69: 📐 Maintainability & Code Quality | 🔵 TrivialMake
setAllowEmptyEndpointsreturnCentralDogmaEndpointGroupBuilder<T>for a consistent fluent API.The
selectionStrategy()method (line 48) returns the builder for method chaining, butsetAllowEmptyEndpoints()(line 67) returnsvoid, breaking the fluent API pattern. Returningthiswould allow seamless chaining with other builder methods.♻️ Proposed change
- public void setAllowEmptyEndpoints(boolean allowEmptyEndpoints) { + public CentralDogmaEndpointGroupBuilder<T> setAllowEmptyEndpoints(boolean allowEmptyEndpoints) { this.allowEmptyEndpoints = allowEmptyEndpoints; + return this; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroupBuilder.java` around lines 67 - 69, The setAllowEmptyEndpoints method currently returns void, which breaks the fluent API pattern used elsewhere in the CentralDogmaEndpointGroupBuilder class. Change the return type of setAllowEmptyEndpoints from void to CentralDogmaEndpointGroupBuilder<T> to match the pattern used in selectionStrategy(), and add a return statement at the end of the method that returns this to enable method chaining consistency with other builder methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroupBuilder.java`:
- Around line 67-69: The setAllowEmptyEndpoints method currently returns void,
which breaks the fluent API pattern used elsewhere in the
CentralDogmaEndpointGroupBuilder class. Change the return type of
setAllowEmptyEndpoints from void to CentralDogmaEndpointGroupBuilder<T> to match
the pattern used in selectionStrategy(), and add a return statement at the end
of the method that returns this to enable method chaining consistency with other
builder methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c406da3-2ed4-4062-9478-c9b7d3e0eee3
📒 Files selected for processing (2)
client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.javaclient/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroupBuilder.java
💤 Files with no reviewable changes (1)
- client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java
4 participants
We have a use case where this is desirable for a testing-only endpoint group (which will be empty when there are no tests currently active, and will then be populated before testing starts - we'd like the server to start up successfully even if no tests are currently active).