dimas-b commented on code in PR #4115:
URL: https://github.com/apache/polaris/pull/4115#discussion_r3414278725
##########
polaris-core/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:
`IcebergMetricsReporter` is NOT used in `polaris-core`.
From a general design perspective, I do not see a reason for defining an SPI
class in `polaris-core` when `polaris-core` does not actually use that
functionality.
`polaris-core` is a historical artifact and I do not think it was ever
intended as a container for SPI classes.
`IcebergMetricsReporter` is actually called from `IcebergCatalogHandler`.
So, it represents the "needs" of the `runtime/services` module in terms of
metrics processing capabilities required by the REST API layer implemented in
that module, which is exactly what SPI stands for.
If we want to establish / clarify our general guidelines for the location of
SPI classes, I suggest starting a dedicated discussion on the `dev` ML.
As an incremental step, I believe adding a new module called
`extensions/metrics-reports/spi` for this class could be a reasonable starting
point. The `polaris-core` module will _not_ depend on it. The `runtime/service`
module and the impl. module _will_ depend on it.
I would not attach too much meaning to the term "extensions" in this
context. It does not mean that `runtime` cannot depend on "extensions", IMHO. I
believe "extensions" in this repo is merely a container for plugins, offering
different impl. options for some Polaris features. The naming of these modules
is part of what, I believe, should be discussed on `dev` (in a non-blocking
manner WRT this PR).
Keeping new SPI classes isolated from the start will simplify future
refactoring after we reach consensus on `dev` about general SPI placement
guidelines.
##########
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:
Sorry, I missed this before... Does `runtime/service` need this dep? My
local build was ok without it 😅 (but I did not run tests).
##########
runtime/server/build.gradle.kts:
##########
@@ -38,6 +38,7 @@ dependencies {
runtimeOnly("io.quarkus:quarkus-jdbc-postgresql")
runtimeOnly(project(":polaris-extensions-federation-hadoop"))
runtimeOnly(project(":polaris-extensions-auth-opa"))
+ runtimeOnly(project(":polaris-extensions-metrics-reports"))
Review Comment:
just for clarify: this dep is perfectly fine from my POV as it makes the ASF
Polaris Server build include the Metrics read endpoint(s), which is intended.
Downstream servers should not be required to include the Metrics endpoint
(the concern about the `runtime/service` dep. change.
--
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]
