You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "HTHou (via GitHub)" <gi...@apache.org> on 2026/09/23 17:15:57 UTC
[PR] Replace Reactor Netty in Prometheus reporter [iotdb]
HTHou opened a new pull request, #18708:
URL: https://github.com/apache/iotdb/pull/18708
## Summary
- replace Reactor Netty with the JDK `HttpServer`/`HttpsServer` for the Prometheus endpoint
- preserve Basic authentication, TLS/mTLS, asynchronous snapshots, and reporter restart behavior
- remove the Reactor/Netty dependencies from `metrics-interface` and declare the customized MQTT example direct `netty-buffer` dependency
- reduce the all distribution from 105,334,150 to 99,144,120 bytes and from 97 to 77 jars
## Testing
- `mvn test -pl iotdb-core/metrics/interface`
- `mvn package -pl distribution -am -DskipTests`
- `mvn test-compile -DskipTests`
- `mvn test-compile -P with-zh-locale -DskipTests`
- `mvn spotless:check -pl iotdb-core/metrics/interface,iotdb-core/node-commons,example/mqtt-customize`
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Reactor Netty in Prometheus reporter [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18708:
URL: https://github.com/apache/iotdb/pull/18708#discussion_r4085278006
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/reporter/prometheus/PrometheusReporter.java:
##########
@@ -189,9 +154,68 @@ public synchronized boolean start() {
return true;
}
+ private HttpServer createHttpServer() throws Exception {
+ InetSocketAddress address = new InetSocketAddress(METRIC_CONFIG.getPrometheusReporterPort());
+ if (!METRIC_CONFIG.isEnableSSL()) {
+ return HttpServer.create(address, 0);
+ }
+
+ SSLContext sslContext =
+ createSslContext(
+ METRIC_CONFIG.getKeyStorePath(),
+ METRIC_CONFIG.getKeyStorePassword(),
+ METRIC_CONFIG.getTrustStorePath(),
+ METRIC_CONFIG.getTrustStorePassword());
+ HttpsServer httpsServer = HttpsServer.create(address, 0);
+ httpsServer.setHttpsConfigurator(
+ new HttpsConfigurator(sslContext) {
+ @Override
+ public void configure(HttpsParameters parameters) {
+ SSLParameters sslParameters = sslContext.getDefaultSSLParameters();
+ sslParameters.setNeedClientAuth(true);
+ parameters.setSSLParameters(sslParameters);
+ }
+ });
+ return httpsServer;
+ }
+
+ private ExecutorService newHttpExecutor() {
+ return Executors.newFixedThreadPool(
+ PROMETHEUS_HTTP_SERVER_WORKER_THREADS,
Review Comment:
This two-thread pool services both synchronous metric collection and blocking socket writes. Two slow/non-reading clients can occupy both workers in `OutputStream.write`, while every later Prometheus scrape accumulates in the fixed pool's unbounded queue; this regresses the nonblocking Reactor endpoint into an easily stalled endpoint. Please use a server/executor strategy with bounded queuing and request/write timeouts (and enough isolation for scrapes).
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/concurrent/ThreadName.java:
##########
@@ -192,9 +192,7 @@ public enum ThreadName {
SYSTEM_SCHEDULE_METRICS("SystemScheduleMetrics"),
RESOURCE_CONTROL_DISK_STATISTIC("ResourceControl-DataRegionDiskStatistics"),
PROMETHEUS_REPORTER_SNAPSHOT_UPDATER("PrometheusReporter-Snapshot-Updater"),
- PROMETHEUS_REACTOR_HTTP_EPOLL("reactor-http-epoll"),
- PROMETHEUS_REACTOR_HTTP_NIO("reactor-http-nio"),
- PROMETHEUS_BOUNDED_ELASTIC("boundedElastic-evictor"),
+ PROMETHEUS_REPORTER_HTTP("prometheus-reporter-http"),
Review Comment:
`HttpServer.start()` also creates an internal thread named `HTTP-Dispatcher`; that thread does not use the supplied executor/thread factory. Registering only `prometheus-reporter-http` therefore leaves part of the new Prometheus server classified as `UNKNOWN` by `ThreadName`, so thread-pool/module metrics become incomplete. Please classify the dispatcher thread as metrics as well (or provide another way to account for it).
##########
iotdb-core/metrics/interface/src/test/java/org/apache/iotdb/metrics/reporter/prometheus/PrometheusReporterTest.java:
##########
@@ -36,6 +38,31 @@
public class PrometheusReporterTest {
+ @Test
+ public void testMetricsEndpoint() throws Exception {
+ MetricConfig metricConfig = MetricConfigDescriptor.getInstance().getMetricConfig();
+ boolean originalAsyncUpdate = metricConfig.isPrometheusReporterAsyncUpdate();
+ Integer originalPort = metricConfig.getPrometheusReporterPort();
+ metricConfig.setPrometheusReporterAsyncUpdate(false);
+ metricConfig.setPrometheusReporterPort(0);
+
+ PrometheusReporter reporter = new PrometheusReporter(new DoNothingMetricManager());
+ try {
+ assertTrue(reporter.start());
+ HttpURLConnection connection =
+ (HttpURLConnection)
+ new URL("http://127.0.0.1:" + reporter.getListeningPort() + "/metrics")
+ .openConnection();
+ connection.setRequestMethod("GET");
+ assertEquals(200, connection.getResponseCode());
+ assertEquals("text/plain", connection.getHeaderField("Content-Type"));
Review Comment:
The replacement test covers only unauthenticated plaintext HTTP, although this change rewrites the Basic-auth path and the entire TLS/mTLS setup. Add endpoint tests for unauthorized/authorized Basic requests and for a trusted versus untrusted client certificate so the behaviors called out as preserved cannot silently regress.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Reactor Netty in Prometheus reporter [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18708:
URL: https://github.com/apache/iotdb/pull/18708#discussion_r4085398718
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/concurrent/ThreadName.java:
##########
@@ -394,10 +393,8 @@ public enum ThreadName {
SYSTEM_SCHEDULE_METRICS,
RESOURCE_CONTROL_DISK_STATISTIC,
PROMETHEUS_REPORTER_SNAPSHOT_UPDATER,
- PROMETHEUS_REACTOR_HTTP_EPOLL,
- PROMETHEUS_REACTOR_HTTP_NIO,
- PROMETHEUS_REACTOR_HTTP_EPOLL,
- PROMETHEUS_BOUNDED_ELASTIC));
+ PROMETHEUS_REPORTER_HTTP,
+ PROMETHEUS_HTTP_DISPATCHER));
Review Comment:
These new threads are not actually attributed to the metrics module. `threadNameSetList` includes `iotConsensusV2ThreadNames` before `ratisThreadNames` (lines 413–427), but the parallel `modules` array has no corresponding second `IOT_CONSENSUS` entry (lines 439–461), so `metricsThreadNames` is paired with `ThreadModule.RPC`. Consequently both Prometheus threads are reported as RPC by `getModuleTheThreadBelongs`, contrary to the intended classification. Add the missing module entry (and preferably a mapping test) so the arrays remain aligned.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Reactor Netty in Prometheus reporter [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #18708:
URL: https://github.com/apache/iotdb/pull/18708#discussion_r4085434562
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/concurrent/ThreadName.java:
##########
@@ -394,10 +393,8 @@ public enum ThreadName {
SYSTEM_SCHEDULE_METRICS,
RESOURCE_CONTROL_DISK_STATISTIC,
PROMETHEUS_REPORTER_SNAPSHOT_UPDATER,
- PROMETHEUS_REACTOR_HTTP_EPOLL,
- PROMETHEUS_REACTOR_HTTP_NIO,
- PROMETHEUS_REACTOR_HTTP_EPOLL,
- PROMETHEUS_BOUNDED_ELASTIC));
+ PROMETHEUS_REPORTER_HTTP,
+ PROMETHEUS_HTTP_DISPATCHER));
Review Comment:
Fixed in 10c5dd3c12 by adding the missing second IOT_CONSENSUS entry so threadNameSetList and modules remain aligned. I also added ThreadNameTest covering IoTConsensusV2, Ratis, Metrics, RPC, and Other mappings.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Reactor Netty in Prometheus reporter [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #18708:
URL: https://github.com/apache/iotdb/pull/18708#discussion_r4085354504
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/reporter/prometheus/PrometheusReporter.java:
##########
@@ -189,9 +154,68 @@ public synchronized boolean start() {
return true;
}
+ private HttpServer createHttpServer() throws Exception {
+ InetSocketAddress address = new InetSocketAddress(METRIC_CONFIG.getPrometheusReporterPort());
+ if (!METRIC_CONFIG.isEnableSSL()) {
+ return HttpServer.create(address, 0);
+ }
+
+ SSLContext sslContext =
+ createSslContext(
+ METRIC_CONFIG.getKeyStorePath(),
+ METRIC_CONFIG.getKeyStorePassword(),
+ METRIC_CONFIG.getTrustStorePath(),
+ METRIC_CONFIG.getTrustStorePassword());
+ HttpsServer httpsServer = HttpsServer.create(address, 0);
+ httpsServer.setHttpsConfigurator(
+ new HttpsConfigurator(sslContext) {
+ @Override
+ public void configure(HttpsParameters parameters) {
+ SSLParameters sslParameters = sslContext.getDefaultSSLParameters();
+ sslParameters.setNeedClientAuth(true);
+ parameters.setSSLParameters(sslParameters);
+ }
+ });
+ return httpsServer;
+ }
+
+ private ExecutorService newHttpExecutor() {
+ return Executors.newFixedThreadPool(
+ PROMETHEUS_HTTP_SERVER_WORKER_THREADS,
Review Comment:
Addressed in 1198beaf5c: the executor now scales from 2 to 8 workers and uses a bounded queue of 16 with AbortPolicy, so two slow clients no longer starve later scrapes and requests cannot accumulate in an unbounded queue. JDK HttpServer does not expose a supported per-response socket timeout; the bounded executor limits the remaining exposure.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/concurrent/ThreadName.java:
##########
@@ -192,9 +192,7 @@ public enum ThreadName {
SYSTEM_SCHEDULE_METRICS("SystemScheduleMetrics"),
RESOURCE_CONTROL_DISK_STATISTIC("ResourceControl-DataRegionDiskStatistics"),
PROMETHEUS_REPORTER_SNAPSHOT_UPDATER("PrometheusReporter-Snapshot-Updater"),
- PROMETHEUS_REACTOR_HTTP_EPOLL("reactor-http-epoll"),
- PROMETHEUS_REACTOR_HTTP_NIO("reactor-http-nio"),
- PROMETHEUS_BOUNDED_ELASTIC("boundedElastic-evictor"),
+ PROMETHEUS_REPORTER_HTTP("prometheus-reporter-http"),
Review Comment:
Addressed in 1198beaf5c by classifying the JDK HttpServer HTTP-Dispatcher thread under the metrics module in addition to the configured worker threads.
##########
iotdb-core/metrics/interface/src/test/java/org/apache/iotdb/metrics/reporter/prometheus/PrometheusReporterTest.java:
##########
@@ -36,6 +38,31 @@
public class PrometheusReporterTest {
+ @Test
+ public void testMetricsEndpoint() throws Exception {
+ MetricConfig metricConfig = MetricConfigDescriptor.getInstance().getMetricConfig();
+ boolean originalAsyncUpdate = metricConfig.isPrometheusReporterAsyncUpdate();
+ Integer originalPort = metricConfig.getPrometheusReporterPort();
+ metricConfig.setPrometheusReporterAsyncUpdate(false);
+ metricConfig.setPrometheusReporterPort(0);
+
+ PrometheusReporter reporter = new PrometheusReporter(new DoNothingMetricManager());
+ try {
+ assertTrue(reporter.start());
+ HttpURLConnection connection =
+ (HttpURLConnection)
+ new URL("http://127.0.0.1:" + reporter.getListeningPort() + "/metrics")
+ .openConnection();
+ connection.setRequestMethod("GET");
+ assertEquals(200, connection.getResponseCode());
+ assertEquals("text/plain", connection.getHeaderField("Content-Type"));
Review Comment:
Added endpoint coverage for both unauthorized and authorized Basic-auth requests in 1198beaf5c. TLS/mTLS setup still lacks a focused endpoint test because the metrics-interface module currently has no certificate fixtures; the JDK SSLContext path is exercised by compilation but not a handshake test in this 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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org