Remove PolarisConfiguration.loadConfig #1356
Conversation
eric-maynard
requested review from
adutra,
ashvina,
dennishuo and
dimas-b
as code owners
eric-maynard
requested review from
MonkeyCanCode,
RussellSpitzer,
collado-mike,
ebyhr,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public synchronized StorageCredentialCache getOrCreateStorageCredentialCache( | ||
| RealmContext realmContext) { | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext) { |
There was a problem hiding this comment.
You can get from PolarisCallContext to RealmContext, so I don't see a point in passing in two arguments
There was a problem hiding this comment.
You can get from PolarisCallContext to RealmContext
Is that true? I think you can get from CallContext to RealmContext, but not from PolarisCallContext. Personally I would like to unify these in the future but I want to keep this PR tightly scoped.
| // by name cache | ||
| this.byName = new ConcurrentHashMap<>(); | ||
|
|
||
| this.polarisCallContext = polarisCallContext; |
There was a problem hiding this comment.
EntityCache can't have a PolarisCallContext in its constructor and certainly not as an instance field. The cache lives beyond a single call, so only long lived contexts should be referenced. Any number of things can change between calls, even things that might change the configuration store's response.
There was a problem hiding this comment.
This particular case is worth discussing more. For things like the EntityCache, it may be the case that there are things you can configure in the application.properties which are fundamentally invalid to set at "request time". They may have to do with how Polaris starts up, or they may affect a singleton(-ish) object like the EntityCache.
Currently we still just use the config via the PolarisCallContext with the acknowledgement that it's only really used once and then ignored in future requests. Is there a better pattern we can use here for accessing configs?
There was a problem hiding this comment.
cc @gh-yzou who I believe is working on an application-scoped feature store
|
|
||
| long weigherTarget = | ||
| PolarisConfiguration.loadConfig(FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET); | ||
| polarisCallContext |
There was a problem hiding this comment.
For cases like this, either the configuration store needs non-realm-specific config APIs or we should construct an EntityCacheConfig object at startup that users can set using the application.properties file.
There was a problem hiding this comment.
the configuration store needs non-realm-specific config APIs
Agreed. Notably, this doesn't work as written on main today. So this isn't a new problem introduced by the PR.
Having said that, we should fix this and I would be okay fixing this in this PR. Do you have a way you'd like to model this?
|
|
||
| /** Initialize the creds cache */ | ||
| public StorageCredentialCache() { | ||
| public StorageCredentialCache(PolarisCallContext polarisCallContext) { |
There was a problem hiding this comment.
Same comments re: the EntityCache
|
|
||
| EntityCache getOrCreateEntityCache(RealmContext realmContext); | ||
| EntityCache getOrCreateEntityCache( | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext); |
There was a problem hiding this comment.
I do not support passing context parameter as method args in general and this PR adds another context parameter.
Could we leverage CDI for injecting context data?
There was a problem hiding this comment.
This is a good callout. In general I agree that we can leverage CDI to inject context parameters (e.g. PolarisCallContext) around. However in this particular case, the EntityCache is not a CDI-managed class. There's a discussion above about this too, but I think we need a way to have configs that are not tied to a PolarisCallContext('s configuration store).
| if (!storageCredentialCacheMap.containsKey(realmContext.getRealmIdentifier())) { | ||
| storageCredentialCacheMap.put( | ||
| realmContext.getRealmIdentifier(), new StorageCredentialCache()); | ||
| realmContext.getRealmIdentifier(), new StorageCredentialCache(polarisCallContext)); |
There was a problem hiding this comment.
This look strange to me. We cache StorageCredentialCache only by realm ID, but the instance references a potentially old PolarisCallContext. Can we be sure that PolarisCallContext inside the StorageCredentialCache is still relevant when it is obtained from the map later?
Currently we only use polarisCallContext for resolving config parameters, still, it is very non-intuitive that the config values resolved in one polarisCallContext are applicable to another polarisCallContext even within the same realm 🤔
| long weigherTarget = | ||
| PolarisConfiguration.loadConfig(FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET); | ||
| polarisCallContext | ||
| .getConfigurationStore() |
There was a problem hiding this comment.
I looked through the code and as far as I can see all instances of PolarisConfigurationStore happen to be application-scoped. Could we avoid requiring a "call" (i.e. request) context for getting PolarisConfigurationStore?
If injection through CDI seems too intrusive for this PR, could we instead use PolarisConfigurationStore as the new parameter (instead of PolarisCallContext)?
There was a problem hiding this comment.
but TBH, I'd really like to use CDI (with custom producers) for injection rather than relying on factories in the main call path. In other words, I'd like the main body of code to receive dependent objects through injection and factories / producers to be removed from the main call paths and be engaged by CDI when request-handlers are constructed.
All-in-all, the intention behind this PR seems to be pointing in that direction, so why not do the CDI-based refactoring now and then perhaps the config loading issue is solved automatically?
|
Holding this for #1505 |
github-project-automation
bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
3 participants
PolarisConfiguration.loadConfigrelies onCallContext.getCurrentContextwhich may not be apparent to callers and often will not work. If the CallContext ThreadLocal hasn't been appropriately set in the calling thread, this method will return invalid results. This PR proposed to remove the problematic method as part of a series of changes to reduce or remove the usage of ThreadLocal throughout Polaris.