dimas-b commented on code in PR #4117: URL: https://github.com/apache/polaris/pull/4117#discussion_r3033660138
########## polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java: ########## @@ -16,12 +16,12 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.polaris.service.context.catalog; +package org.apache.polaris.core.catalog; Review Comment: > It’s because the interface itself is a contract/SPI boundary, and that contract is expressed entirely in core types (PolarisResolutionManifest). I agree that this interface is an SPI, however, it is not a "core SPI" in the sense that Polaris Core does not need an implementation of this SPI to be provided in order to function property. If the idea behind moving it to `polaris-core` is only to consolidate all SPI classes in `polaris-core`, I'd like to propose postponing the move until we make progress on the general SPI standardization discussion (which we started on `dev` a long time ago, but did not progress, unfortunately). I imagine different components of Polaris may have different SPI surfaces. In this case `LocalCatalogFactory` is required by the `service` code, so keeping it under `runtime/service` seems more natural to me. If we were to formalize service SPI, I'd suggest adding a dedicated `runtime/service-spi` module instead of moving to `polaris-core`. That would lead to better isolation and dependency coherence among modules. My view of SPI is that it should be strongly associated with the module that calls it in runtime, not with the module that defines its method parameter types. In any case, as I noted above, I hope for some consensus on the SPI standardization discussion before moving classes across modules. -- 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]
