Discover additional flag definitions via ServiceLoader SPI#36603
Closed
Discover additional flag definitions via ServiceLoader SPI#36603
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a ServiceLoader-based extension point so external artifacts on the classpath can contribute additional flag definitions to the central Flags registry, with tests verifying discovery and interaction with Flags.Replacer.
Changes:
- Introduce
FlagDefinitionsSPI (singleregister()method) for out-of-bundle flag registration. - Load and invoke
FlagDefinitionsproviders duringFlagsclass initialization (and trigger fromPermanentFlags). - Add test-only SPI implementation + ServiceLoader registration and regression tests for visibility and
Flags.Replacerbehavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| flags/src/main/java/com/yahoo/vespa/flags/FlagDefinitions.java | Adds the SPI contract for registering external flag definitions. |
| flags/src/main/java/com/yahoo/vespa/flags/Flags.java | Loads FlagDefinitions via ServiceLoader once and runs providers during static init. |
| flags/src/main/java/com/yahoo/vespa/flags/PermanentFlags.java | Calls into Flags to ensure SPI loading when PermanentFlags is initialized. |
| flags/src/test/resources/META-INF/services/com.yahoo.vespa.flags.FlagDefinitions | Registers the test SPI implementation on the test classpath. |
| flags/src/test/java/com/yahoo/vespa/flags/TestFlagDefinitions.java | Test-only FlagDefinitions provider that registers sample flags. |
| flags/src/test/java/com/yahoo/vespa/flags/FlagDefinitionsSpiTest.java | Verifies ServiceLoader discovery and SPI-registered flags are visible; checks no double-registration. |
| flags/src/test/java/com/yahoo/vespa/flags/FlagsReplacerSpiRegressionTest.java | Regression tests ensuring Flags.Replacer saves/restores SPI-registered flags correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add a FlagDefinitions service-provider interface so downstream artifacts can register flags into Flags/PermanentFlags without depending on this bundle at compile time. Flags.<clinit> (and, defensively, PermanentFlags. <clinit>) load implementations via java.util.ServiceLoader and invoke register() at most once per JVM, after the static flag map and all in-bundle flag fields are initialized.
f563d64 to
a077281
Compare
Contributor
Author
|
As discussed, let's find another way. |
This file contains hidden or 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
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.
What
Add a
FlagDefinitionsservice-provider interface (singleregister()method).
Flags.<clinit>andPermanentFlags.<clinit>discoverimplementations via
java.util.ServiceLoaderand invokeregister()once per JVM, after the static flag map is initialized. Existing flag
definitions are untouched.
Why
modifying this one.
out of this generic bundle.
to any other artifact, and no behavior change for flags currently
defined here.