You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anoopj (via GitHub)" <gi...@apache.org> on 2025/07/09 23:47:05 UTC
[PR] Make metrics reporting asynchronous [iceberg]
anoopj opened a new pull request, #13507:
URL: https://github.com/apache/iceberg/pull/13507
Right now the implementation of RESTMetricsReporter is synchronous and the REST call can take a significant amount of time. Change the reporting to be async instead.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2198883672
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +53,17 @@ public void report(MetricsReport report) {
return;
}
- try {
- client.post(
- metricsEndpoint,
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
- } catch (Exception e) {
- LOG.warn("Failed to report metrics to REST endpoint {}", metricsEndpoint, e);
- }
+ Tasks.range(1)
+ .executeWith(ThreadPools.getWorkerPool())
Review Comment:
@amogh-jahagirdar Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2196213424
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +56,30 @@ public void report(MetricsReport report) {
return;
}
+ executor.submit(() -> {
Review Comment:
I would suggest that we use `Tasks` utility class here. I believe the rest client will handle a number of retry scenarios, so I don't know how much we want to do retries at this layer. @nastra might have thoughts on it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2199700864
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +57,20 @@ public void report(MetricsReport report) {
return;
}
- try {
- client.post(
- metricsEndpoint,
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
- } catch (Exception e) {
- LOG.warn("Failed to report metrics to REST endpoint {}", metricsEndpoint, e);
- }
+ Tasks.range(1)
+ .executeWith(METRICS_EXECUTOR)
Review Comment:
we should also set `.suppressFailureWhenFinished()`, otherwise the exception is going to be propagated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #13507:
URL: https://github.com/apache/iceberg/pull/13507
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2199017447
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -53,6 +53,17 @@ private SystemConfigs() {}
Math.max(2, 4 * Runtime.getRuntime().availableProcessors()),
Integer::parseUnsignedInt);
+ /**
+ * Sets the size of the metrics reporting thread pool. This limits the number of concurrent
+ * metrics reporting operations.
+ */
+ public static final ConfigEntry<Integer> METRICS_THREAD_POOL_SIZE =
+ new ConfigEntry<>(
+ "iceberg.metrics.num-threads",
+ "ICEBERG_METRICS_NUM_THREADS",
+ 2,
+ Integer::parseUnsignedInt);
+
Review Comment:
Makes sense. Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2197772225
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +56,30 @@ public void report(MetricsReport report) {
return;
}
+ executor.submit(() -> {
Review Comment:
My gut feeling is that we shouldn't be retrying here. We don't make any guarantees about metrics reports and we don't want to cause/surface any issues in case metrics can't be sent
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2197772225
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +56,30 @@ public void report(MetricsReport report) {
return;
}
+ executor.submit(() -> {
Review Comment:
My gut feeling is that we shouldn't be retrying at all here. We don't make any guarantees about metrics reports and we don't want to cause any issues in case metrics can't be sent
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2200847375
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +57,20 @@ public void report(MetricsReport report) {
return;
}
- try {
- client.post(
- metricsEndpoint,
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
- } catch (Exception e) {
- LOG.warn("Failed to report metrics to REST endpoint {}", metricsEndpoint, e);
- }
+ Tasks.range(1)
+ .executeWith(METRICS_EXECUTOR)
Review Comment:
Good catch @nastra
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2196210942
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -36,12 +39,14 @@ class RESTMetricsReporter implements MetricsReporter {
private final RESTClient client;
private final String metricsEndpoint;
private final Supplier<Map<String, String>> headers;
+ private final ExecutorService executor;
Review Comment:
You might want to take a look at the `ThreadPools` utility class/usages. We probably want an exiting pool and I'm not sure if we should just use the worker pool.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2198085378
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -36,12 +39,14 @@ class RESTMetricsReporter implements MetricsReporter {
private final RESTClient client;
private final String metricsEndpoint;
private final Supplier<Map<String, String>> headers;
+ private final ExecutorService executor;
Review Comment:
Done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2198085990
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +56,30 @@ public void report(MetricsReport report) {
return;
}
+ executor.submit(() -> {
Review Comment:
Sounds good. Not adding any retry.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2198843241
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +53,17 @@ public void report(MetricsReport report) {
return;
}
- try {
- client.post(
- metricsEndpoint,
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
- } catch (Exception e) {
- LOG.warn("Failed to report metrics to REST endpoint {}", metricsEndpoint, e);
- }
+ Tasks.range(1)
+ .executeWith(ThreadPools.getWorkerPool())
Review Comment:
I don't think we should be using the worker pool here since that's used for planning operations. I wouldn't want to possibly interfere with that with metrics reporting which is basically best effort. Should we just define an exiting thread pool `metrics-publisher-pool` or something like that and then just use that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2198953282
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -53,6 +53,17 @@ private SystemConfigs() {}
Math.max(2, 4 * Runtime.getRuntime().availableProcessors()),
Integer::parseUnsignedInt);
+ /**
+ * Sets the size of the metrics reporting thread pool. This limits the number of concurrent
+ * metrics reporting operations.
+ */
+ public static final ConfigEntry<Integer> METRICS_THREAD_POOL_SIZE =
+ new ConfigEntry<>(
+ "iceberg.metrics.num-threads",
+ "ICEBERG_METRICS_NUM_THREADS",
+ 2,
+ Integer::parseUnsignedInt);
+
Review Comment:
Sorry I should've been more specific on my last review, while I think the pool should be separate I don't think we need the metrics thread pool to be configurable at least at this point (I also think we could probably get away with a threadpool size of 1 for this but that's a minor point). I don't think it really generalizes beyond the REST case at least at the moment, so until it does we should probably keep it isolated is my opinion.
TLDR: I think we should just have a static field `Threadpools.newExitingWorkerPool("rest-metrics-reporter", someFixedNumberOfThreads)` inside `RESTMetricReporter`
##########
core/src/main/java/org/apache/iceberg/util/ThreadPools.java:
##########
@@ -51,6 +51,11 @@ private ThreadPools() {}
private static final ExecutorService DELETE_WORKER_POOL =
newExitingWorkerPool("iceberg-delete-worker-pool", DELETE_WORKER_THREAD_POOL_SIZE);
+ public static final int METRICS_THREAD_POOL_SIZE = SystemConfigs.METRICS_THREAD_POOL_SIZE.value();
Review Comment:
See comment above, I don't think we really need to be exposing this since it's specific to the REST metrics reporter implementation. I think it makes the most sense just to have a static threadpool field in RESTMetricsREporter
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make metrics reporting asynchronous [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #13507:
URL: https://github.com/apache/iceberg/pull/13507#discussion_r2200805021
##########
core/src/main/java/org/apache/iceberg/rest/RESTMetricsReporter.java:
##########
@@ -51,15 +57,20 @@ public void report(MetricsReport report) {
return;
}
- try {
- client.post(
- metricsEndpoint,
- ReportMetricsRequest.of(report),
- null,
- headers,
- ErrorHandlers.defaultErrorHandler());
- } catch (Exception e) {
- LOG.warn("Failed to report metrics to REST endpoint {}", metricsEndpoint, e);
- }
+ Tasks.range(1)
+ .executeWith(METRICS_EXECUTOR)
Review Comment:
@nastra great callout! Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org