dennishuo commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r3030779532
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java:
##########
@@ -317,31 +313,6 @@ public void deletePrincipalSecrets(
throw illegalMethodError("loadTasks");
}
- @Override
- public @Nonnull ScopedCredentialsResult getSubscopedCredsForEntity(
Review Comment:
Thanks for the ping! Yes, I agree with @dimas-b that this is a sensitive API
surface. Though we should indeed be able to *decouple* the various interfaces
pulled in/aggregated by `PolarisMetaStoreManager`:
PolarisSecretsManager,
PolarisGrantManager,
PolarisCredentialVendor,
PolarisPolicyMappingManager,
PolarisEventManager,
PolarisMetricsManager
those interfaces were specifically written as part of SPIs, and at least the
same *functionality* provided by those extensibility points needs to be
preserved more strictly than syntax evolution of those interfaces, unless a
much deeper attempt is made amongst Polaris users to agree on a migration path
off of such functionality.
There's some discussion about the list of core SPIs in here:
https://lists.apache.org/thread/0nj24zro7kyctqfnlml08ppo7zs9xcqs
The way that applies to this PR is that the interaction with the persistence
layer is intentional, especially for the TransactionalMetaStoreManager path.
The basic use case if if a custom Polaris deployment uses subclasses that
leverage those SPIs in order to perform additional book-keeping around
storage-integration functionality and needs that to be transactional with
entity creation/updates. This is best highlighted by thinking about the
interaction between the subset of `PolarisCredentialVendor` method
implementations with `IntegrationPersistence` which has these methods:
createStorageIntegration
persistStorageIntegrationIfNeeded
loadPolarisStorageIntegration
These are in the interface specifically to define the SPI by which users
customize a "lease, commit, use" model for secondary metadata necessary for
StorageIntegrations. In this current PR, it's not clear what happens to those
methods now -- will `IntegrationPersistence::loadPolarisStorageIntegration`
just be ignored?
Overall, I think the refactoring on the credential caching layer should
still be possible, while preserving the `PolarisCredentialVendor` interface so
that implementations of `PolarisCredentialVendor` still work correctly.
There are some nuances to this that may be better discussed on the mailing
list thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
