Copilot commented on code in PR #4841:
URL: https://github.com/apache/polaris/pull/4841#discussion_r3442314548
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java:
##########
@@ -52,6 +56,10 @@ public URI managementApiEndpoint() {
return baseUri.resolve(baseUri.getRawPath() +
"/api/management").normalize();
}
+ public URI metricsApiEndpoint() {
+ return managementUri.resolve("q/metrics").normalize();
+ }
Review Comment:
`metricsApiEndpoint()` uses `managementUri.resolve("q/metrics")`, which will
drop the last path segment when `managementUri` does not end with `/` (e.g.
`http://host/management` resolves to `http://host/q/metrics`). This can produce
an incorrect metrics URL for management URIs that include a path prefix.
##########
integration-tests/src/test/java/org/apache/polaris/service/it/env/PolarisApiEndpointsTest.java:
##########
@@ -27,7 +27,11 @@ public class PolarisApiEndpointsTest {
@Test
void testEndpointRespectsPathPrefix() {
PolarisApiEndpoints endpoints =
- new PolarisApiEndpoints(URI.create("http://myserver.com/polaris";), "",
"Polaris-Realm");
+ new PolarisApiEndpoints(
+ URI.create("http://myserver.com/polaris";),
+ URI.create("http://myserver.com/management";),
+ "",
+ "Polaris-Realm");
Review Comment:
This unit test covers the catalog/management API endpoints with a path
prefix, but it doesn't assert the newly-added `metricsApiEndpoint()` behavior.
Adding an assertion here would catch incorrect `URI.resolve(...)` behavior when
the management URI includes a path (e.g. `/management`).
##########
integration-tests/src/main/java/org/apache/polaris/service/it/env/Server.java:
##########
@@ -50,5 +50,13 @@ default Map<String, String> headers() {
*/
URI baseUri();
+ /**
+ * The base URI for the Quarkus management (metrics/health) interface. Not
to be confused with the
+ * Polaris Management API.
+ */
+ default URI managementUri() {
+ throw new UnsupportedOperationException("Management URI not yet
implemented");
+ }
Review Comment:
`PolarisIntegrationTestExtension` now calls `server.managementUri()`
unconditionally. Keeping a default implementation that throws
`UnsupportedOperationException` makes failures runtime-only for any custom
`Server` implementations that haven't been updated. Prefer making this method
abstract so implementers are forced to provide it at compile time (or
alternatively provide a non-throwing default).
--
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]
