HonahX commented on PR #1133: URL: https://github.com/apache/polaris/pull/1133#issuecomment-2715957500
Hi @snazy, thanks for the feedback! > I think the test coverage is not good enough, because I could imagine errors in the Iceberg catalog implementations as soon as there's some policy in there. Could you clarify this concern? The Policy implementation is independent of the Iceberg catalog. It has its own service interface/implementation, handler, and a PolicyCatalog within service-common, all separate from Iceberg. (I realize part of the design doc is outdated—I'll update that section.) > Also the interaction of these new types via the entity manager is untested. I can add more tests in `ResolverTest` and `PolarisTestMetastoreManager` to improve coverage. > Generally, I think it's better to see the whole Policy thing working in a feature branch, which can be a PR in draft state, instead of pushing small bits of rather unused code. I do have a draft PR https://github.com/apache/polaris/pull/1104 that includes all necessary Polaris-core changes for the policy store, including basic classes and the PolicyMappingRecord persistence interfaces/implementations. I can publish a larger draft PR that combines core changes with the service implementation for reference. That said, I’d still recommend reviewing and merging smaller pieces incrementally—this makes it easier for everyone to review and avoids one massive PR that’s harder to manage. Let me know what you 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]
