tokoko commented on PR #3699:
URL: https://github.com/apache/polaris/pull/3699#issuecomment-4229649385
@dennishuo thanks for the explanation. some sort of convergence between
normal credential vending and federation makes sense. I'm not too familiar with
federation codebase so I'll leave that out of scope for now, but I'll keep that
in mind. that would probably make something like #4052 cleaner.
I went ahead and made quite a few changes. technically StorageIntegration
instances returned by `StorageIntegrationProvider` now are storageConfig-scoped
rather than entity-scoped which probably doesn't make much of a difference as
of now. (other than the fact that storage locations need to be passed as
parameters, which they might not have been otherwise, not sure about that
though).
I'm thinking of trimming PolarisStorageIntegration itself next:
- @dimas-b I saw you had previously removed #1480 from Vendor but left it on
the integration layer, any reason for that? from what I saw it's completely
unused and can be deleted from there as well. with that gone
`InMemoryStorageIntegration` class also no longer makes sense to be kept
around. The only active code there is a static method that can be refactored
somewhere else.
- `getStorageIdentifierOrId` is another piece of unused dead code that I
think can be removed.
- On the other hand, I'm thinking of splitting current
`PolarisStorageIntegration` into a slimmer interface and renaming current code
to `CachingStorageIntegration`. I think that should be better for
persistence-based credentials that are unlikely to require caching (?). Also in
general if we're making `PolarisStorageIntegration` a primary SPI, it should
really be an interface.
- All in all, this is the interface I'm going for (also factoring in
@dimas-b's suggestion of passing raw locations and actions rather than separate
read/write locations):
```
public interface PolarisStorageIntegration {
StorageAccessConfig getStorageAccessConfig(
@Nonnull Set<String> locations,
@Nonnull Set<PolarisStorageActions> actions,
@Nonnull Optional<String> refreshEndpoint,
@Nonnull CredentialVendingContext context);
}
```
--
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]
