Refactor PolarisConfigurationStore to streamline value typing#3324
Closed
dimas-b wants to merge 2 commits intoapache:mainfrom
Closed
Refactor PolarisConfigurationStore to streamline value typing#3324dimas-b wants to merge 2 commits intoapache:mainfrom
dimas-b wants to merge 2 commits intoapache:mainfrom
Conversation
snazy
reviewed
Jan 5, 2026
Comment on lines
42
to
45
| * <p>This method is meant to be overridden by concrete configuration store implementations. This | ||
| * method is not meant to be calls by code that needs access to configuration values. | ||
| * Configuration consumers should call typed methods that take a {@link PolarisConfiguration} | ||
| * parameter instead. |
Member
There was a problem hiding this comment.
Suggested change
| * <p>This method is meant to be overridden by concrete configuration store implementations. This | |
| * method is not meant to be calls by code that needs access to configuration values. | |
| * Configuration consumers should call typed methods that take a {@link PolarisConfiguration} | |
| * parameter instead. | |
| * <p>This method is meant to be overridden by concrete configuration store implementations. | |
| * Prefer the typed functions that take a {@link PolarisConfiguration} parameter instead of a {@code String}. |
Contributor
Author
There was a problem hiding this comment.
I reformatted the javadoc, hoping to emphasize the need to use typed methods.
TBH, I'm not sure whether the "prefer" suggestion is targeting callers or implementors 🤔 WDYT?
* Avoid generic return types when the value type is not certain * Add new method to be overridden by implementation that returns `Object` to avoid arbitrary casts to `T` * Type casts are performed only when the expected value type is known. * This change is backward-compatible with older implementations of `PolarisConfigurationStore`
dimas-b
force-pushed
the
more-config-types
branch
from
53dc814 to
a71fbcb
Compare
snazy
reviewed
Jan 5, 2026
| /** | ||
| * Retrieve the current value for a configuration key for a given realm. May be null if not set. | ||
| * | ||
| * <p>This method is meant to be overridden by concrete configuration store implementations. |
Member
There was a problem hiding this comment.
Actually, I wonder whether this function and the getConfiguration functions should have a default implementation at all. Wouldn't it be cleaner to move that to the actual implementation? Mean, this interface isn't really an interface but rather a not-really abstract class.
Contributor
Author
There was a problem hiding this comment.
Good point. I've been thinking about splitting this into an API/SPI pair too, but I thought it might be too intrusive... Let me actually try that refactoring and see what's impacted.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Jan 10, 2026
* Improve code isolation by using `RealmConfig` (like most other code) instead of the lower-level `PolarisConfigurationStore` * This also enabled proper CDI request-scoped injection in concert with apache#3411 * Additionally, this enables further code cleanup in `PolarisConfigurationStore` as discussed in apache#3324 with the goal of using this interface for the backend configuration code, while `RealmConfig` becomes the corresponding frontend interface. * Fix `ServiceProducers.realmConfig()` to make a `RealmConfigImpl` directly from its dependencies (not from `CallContext`) to avoid cycles in CDI.
6 tasks
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Jan 14, 2026
* Improve code isolation by using `RealmConfig` (like most other code) instead of the lower-level `PolarisConfigurationStore` * This also enabled proper CDI request-scoped injection in concert with apache#3411 * Additionally, this enables further code cleanup in `PolarisConfigurationStore` as discussed in apache#3324 with the goal of using this interface for the backend configuration code, while `RealmConfig` becomes the corresponding frontend interface. * Fix `ServiceProducers.realmConfig()` to make a `RealmConfigImpl` directly from its dependencies (not from `CallContext`) to avoid cycles in CDI.
dimas-b
added a commit
to dimas-b/polaris
that referenced
this pull request
Jan 14, 2026
* Improve code isolation by using `RealmConfig` (like most other code) instead of the lower-level `PolarisConfigurationStore` * This also enabled proper CDI request-scoped injection in concert with apache#3411 * Additionally, this enables further code cleanup in `PolarisConfigurationStore` as discussed in apache#3324 with the goal of using this interface for the backend configuration code, while `RealmConfig` becomes the corresponding frontend interface. * Fix `ServiceProducers.realmConfig()` to make a `RealmConfigImpl` directly from its dependencies (not from `CallContext`) to avoid cycles in CDI.
dimas-b
added a commit
that referenced
this pull request
Jan 19, 2026
* Use injected RealmConfig in JdbcMetaStoreManagerFactory * Improve code isolation by using `RealmConfig` (like most other code) instead of the lower-level `PolarisConfigurationStore` * This also enabled proper CDI request-scoped injection in concert with #3411 * Additionally, this enables further code cleanup in `PolarisConfigurationStore` as discussed in #3324 with the goal of using this interface for the backend configuration code, while `RealmConfig` becomes the corresponding frontend interface. * Fix `ServiceProducers.realmConfig()` to make a `RealmConfigImpl` directly from its dependencies (not from `CallContext`) to avoid cycles in CDI.
evindj
pushed a commit
to evindj/polaris
that referenced
this pull request
Jan 26, 2026
* Use injected RealmConfig in JdbcMetaStoreManagerFactory * Improve code isolation by using `RealmConfig` (like most other code) instead of the lower-level `PolarisConfigurationStore` * This also enabled proper CDI request-scoped injection in concert with apache#3411 * Additionally, this enables further code cleanup in `PolarisConfigurationStore` as discussed in apache#3324 with the goal of using this interface for the backend configuration code, while `RealmConfig` becomes the corresponding frontend interface. * Fix `ServiceProducers.realmConfig()` to make a `RealmConfigImpl` directly from its dependencies (not from `CallContext`) to avoid cycles in CDI.
Contributor
Author
|
Closing in favour of #3573 |
github-project-automation
bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
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
2 participants
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.
Avoid generic return types when the value type is not certain
Add new method to be overridden by implementation that returns
Objectto avoid arbitrary casts toTType casts are performed only when the expected value type is known.
This change is backward-compatible with older implementations of
PolarisConfigurationStoreChecklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)