tokoko commented on PR #3699:
URL: https://github.com/apache/polaris/pull/3699#issuecomment-4187429515
@dennishuo @dimas-b I changed the PR and reinstated PolarisCredentialVendor
SPI, but the thing that bothers me now is that
PolarisCredentialVendor.getStorageAccessConfig and
PolarisStorageIntegration.getSubscopedCreds signatures are converging to
essentially the same thing — both take locations, actions, refresh endpoint,
context and return StorageAccessConfig. I think the current design is coupling
PolarisStorageIntegrationProvider (which effectively includes
PolarisStorageIntegration) and PolarisCredentialVendor too much. Having two
SPIs that mirror each other adds complexity without buying us anything.
The root cause is that PolarisCredentialVendor is trying to be a unified SPI
that wraps both "get the right integration" and "vend credentials" into one
call. So what if instead of putting PolarisStorageIntegrationProvider behind
PolarisCredentialVendor, we make PolarisStorageIntegrationProvider a
first-class citizen?
Planned flow will look like this:
Vending:
-
PolarisStorageIntegrationProvider.getStorageIntegration(List<PolarisEntity>
resolvedEntityPath)
- oss: resolves StorageConfig from full entity path and gets cached
per-config PolarisStorageIntegration singleton from the cache.
- custom: gets entity (with findInHierarchy) and calls
IntegrationPersistence.loadPolarisStorageIntegration directly
- StorageIntegration.getSubscopedCreds(...) - since persistence question is
out of the way and PolarisStorageIntegrationProvider SPI gives an implementer
full control over the integration objects that are retrieved, I think we no
longer need another PolarisCredentialVendor SPI for the next step. it would
essentially be identical to PolarisStorageIntegration anyway.
CreateCatalog:
- createStorageIntegration / persistStorageIntegrationIfNeeded:
- oss: we make both a no-op, createStorageIntegration no longer defers
to PolarisStorageIntegrationProvider since PolarisStorageIntegrationProvider
signature has changed and current impl discards the result anyway.
- custom: this is unchanged, implements lease-commit steps here.
This change would give us one SPI (PolarisStorageIntegrationProvider)
instead of two that mirror each other and also keep IntegrationPersistence
storage methods around for custom impls that need them.
Curious what you guys think...
--
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]
