XJDKC commented on PR #1899: URL: https://github.com/apache/polaris/pull/1899#issuecomment-3002308945
> > Given that we may eventually refactor this area to introduce a proper DAO layer, and that we're currently only using the transformation system for a single SigV4-related transformer, it might make sense to proceed with the current approach and revisit the refactor later. WDYT? > > Honestly, I think the approach is too heavyweight and too broad to target a very specific problem. It will be very hard for people to realize (later, by accident) that some entity instance does not reflect the "real" state. That will cause confusion and likely hard to debug issues. > > I propose to separate the relevant parts into APIs/implementations that deal with the problem/use-case at hand, and definitely outside of the metastore - storage credential handling doesn't belong there IMO. Agree with the concern about embedding this logic in the persistence layer, moving it outside does make sense in general as the final entity is different from the entity we pass to the metastore. That said, I want to point out a few things specific to the storage config use case. For storage config, the service identity isn't assigned by Polaris itself, it's provided by users via the `CreateCatalogRequest`. That's why I have this proposal to fix this issue. Also, the storage integration is currently created within the persistence layer: [TransactionalMetaStoreManagerImpl.java#L972-L978](https://github.com/apache/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java#L972-L978). That's why I was leaning toward assigning the service identity in the persistence layer. For certain vendors, we may need to store and retrieve the mapping between realm and service identity info from the database. Doing this lookup in the persistence layer allows us to keep everything within the same transaction when persisting the catalog entity. Moving this from the persistence layer may end up causing inconsistency. Adding some hook points is also very useful in general, e.g. it provides the ability to do some patches if there are some bugs and we end up persisting some wrong data in database. We can also put the service identity injection logic here: [PolarisAdminService.java#L730-L755](https://github.com/apache/polaris/blob/9b5325b3c7f97d5a34057468719eca679b32ac41/service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java#L730-L755) WDYT? @eric-maynard @snazy @dennishuo -- 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]
