obelix74 commented on code in PR #4115:
URL: https://github.com/apache/polaris/pull/4115#discussion_r3415850332
##########
extensions/metrics-reports/spi/src/main/java/org/apache/polaris/core/metrics/IcebergMetricsReporter.java:
##########
@@ -16,38 +16,35 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.reporting;
+package org.apache.polaris.core.metrics;
Review Comment:
Good point. I've extracted `IcebergMetricsReporter` into a new
`extensions/metrics-reports/spi` module
(`polaris-extensions-metrics-reports-spi`). `polaris-core` no longer has any
dependency on or knowledge of this interface. `runtime/service` and the
impl/jdbc extension modules each declare an explicit dep on the new spi module.
I also moved `MetricsRecordConverter` out of `polaris-core` at the same time
— it was only ever called by `PersistingMetricsReporter` in the JDBC extension,
so it now lives there instead.
Agreed on starting a `dev` ML thread to align on general SPI placement
guidelines — I'll keep that scoped separately from this PR.
##########
runtime/service/build.gradle.kts:
##########
@@ -32,6 +32,7 @@ dependencies {
implementation(project(":polaris-api-catalog-service"))
runtimeOnly(project(":polaris-relational-jdbc"))
+ runtimeOnly(project(":polaris-extensions-metrics-reports"))
Review Comment:
You're right to flag it — I've changed this from `runtimeOnly` to
`testRuntimeOnly`. The reporters need to be on the classpath for the
authz/integration tests that exercise the metrics reporting path, but there's
no reason to push them onto the runtime classpath of every downstream server
that depends on `runtime/service`. The `runtime/server` module already has its
own explicit `runtimeOnly` dep for this.
--
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]
