davidradl commented on code in PR #29151:
URL: https://github.com/apache/flink/pull/29151#discussion_r3987508664
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporter.java:
##########
@@ -96,4 +100,26 @@ public void close() {
}
super.close();
}
+
Review Comment:
My AI found:
1. com.sun.net.httpserver.HttpServer in the test
The test uses com.sun.net.httpserver.HttpServer — a JDK internal API. While
it's technically stable and available in JDK 8–21, it's in com.sun.* which
convention discourages. The Flink codebase already has prior usages, so this
likely won't be a blocker, but worth flagging.
2. The null credential cases silently drop auth with no warning
Looking at the test cases ", ,", "user, ,", ", password," — when username or
password is null, the expectedAuthorization is also null (no header is set).
The existing upstream behaviour is preserved here, but there is no log warning
emitted when only one of the two credentials is provided. A half-configured
auth (e.g. username but no password) silently results in unauthenticated
requests. This is arguably a pre-existing issue, but the PR is touching this
code path.
3. Class name shadowing
The new inner class is named BasicAuthHttpConnectionFactory, which is the
same simple name as the upstream class being replaced
(io.prometheus.client.exporter.BasicAuthHttpConnectionFactory). The old import
is removed so there's no actual conflict, but it could cause confusion if the
upstream library ever gets imported again in the future. A name like
JdkBasicAuthHttpConnectionFactory would be clearer.
4. Minor:
assertThat(reporter.getClass().getClassLoader()).isSameAs(classLoader) is
overly strict
This assertion verifies that the reporter class itself was loaded by the
custom classloader. While correct by construction, it's testing implementation
internals of the classloader setup rather than the behaviour under test (auth
header correctness). If the factory loading mechanism ever changes, this
assertion will fail for non-auth-related reasons.
--
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]
