-
Notifications
You must be signed in to change notification settings - Fork 268
Add RealmConfig #2015
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.
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
base: main
Are you sure you want to change the base?
Add RealmConfig #2015
Conversation
febcc48
to
201a495
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is a very nice improvement!
I'd just propose to not unnecessarily repeat "Polaris" in type names.
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisRealmConfig.java
Outdated
Show resolved
Hide resolved
If we're making this change for ergonomic reasons, I don't think the new invocation is really ideal: |
import jakarta.annotation.Nullable; | ||
import org.apache.polaris.core.entity.CatalogEntity; | ||
|
||
/** Realm-specific configuration used to retrieve runtime parameters. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a Realm-specific configuration
? This looks more like a configuration store, like PolarisConfigurationStore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends on one's definition i guess, currently the interface allows retrieval of configuration values. in my book that makes it a "configuration".
do you want me to add "store" to the javadoc or to also rename the class? if the latter, any concrete name suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling it a RealmConfigurationStore would be a better name, but that also only makes me think more that this needs to be reconciled with PolarisConfigurationStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not mind the proposed renaming, but current name LGTM too.
|
||
public class RealmConfigImpl implements RealmConfig { | ||
|
||
private final PolarisConfigurationStore configurationStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a new PolarisConfigurationStore
implementation could achieve the same thing, we should try that approach first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea of having the interface is to make the life of the callers easier as they most often (/always?) operate in the context of a single realm / CallContext
.
whether configuration values are stored by a single store for all/multiple realms or not is an implementation detail that callers should not care about.
so afaict PolarisConfigurationStore
can keep its current implementation unless in the future we notice any advantages of "splitting it up".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whether configuration values are stored by a single store for all/multiple realms or not is an implementation detail that callers should not care about.
Isn't that a reason that you'd want to just inject a PolarisConfigurationStore
rather than handling something called a RealmConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PolarisConfigurationStore
is essentially multi-realm (note its explicit realm method parameters). This class acts as a request-scoped facade to automatically contextualize request-specific config lookup with realm IDs and pass those calls to PolarisConfigurationStore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for refactoring PolarisConfigurationStore
to have both realm-agnostic and realm-gnostic (?) methods. But I do not think a new layer like this over the config store is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new layer helps to properly delineate application-scoped data (PolarisConfigurationStore
and/or it's config source) from request-scoped data (RealmConfig
). Effectively, request-scoped code does not need to deal with explicit realm ID parameters, which, IMHO, is beneficial to code clarity and maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having to deal with realms is definitely a great improvement, agreed. But my concern remains that this new class becomes the de facto “Polaris Configuration Store” and this redundancy does not improve clarity. Furthermore, the proposed structure could be interpreted as implying that these configs are set per-realm (while PolarisConfigurationStore confits are not) when in fact this is not the case.
but you are right, i also wasnt very convinced of the |
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @XN137 ! The PR LGTM overall. I think it's a nice improvement in request/application-scoped delineation in the Polaris codebase.
@eric-maynard : are you ok with merging this PR? |
getting a config value currently requires quite a ceremony:
since a
PolarisConfigurationStore
cant be used without aRealmContext
it makes sense to add a dedicated interface. this allows removal of verbose code and also moves towards injecting that interface via CDI at a request/realm scope in the future.