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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
