dlmarion commented on code in PR #4768:
URL: https://github.com/apache/accumulo/pull/4768#discussion_r1693573846
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReader.java:
##########
@@ -71,9 +72,8 @@ protected TabletServerBatchReader(ClientContext context,
Class<?> scopeClass, Ta
this.tableName = tableName;
this.numThreads = numQueryThreads;
- queryThreadPool =
- context.threadPools().getPoolBuilder("batch scanner " +
batchReaderInstance + "-")
- .numCoreThreads(numQueryThreads).build();
+ queryThreadPool =
context.threadPools().getPoolBuilder(BATCH_SCANNER_POOL_NAME)
Review Comment:
are we losing the batch reader instance in the thread pool name?
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java:
##########
@@ -132,8 +132,9 @@ public CompactionService(String serviceName, String
plannerClass, Long maxRate,
this.executors = Map.copyOf(tmpExecutors);
- this.planningExecutor =
ThreadPools.getServerThreadPools().getPoolBuilder("CompactionPlanner")
- .numCoreThreads(1).numMaxThreads(1).withTimeOut(0L,
MILLISECONDS).build();
+ this.planningExecutor =
+
ThreadPools.getServerThreadPools().getPoolBuilder("compaction.service.compaction.planner")
Review Comment:
No enum for this pool name?
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -229,7 +246,7 @@ public static void resizePool(final ThreadPoolExecutor
pool, final IntSupplier m
*/
public static void resizePool(final ThreadPoolExecutor pool, final
AccumuloConfiguration conf,
final Property p) {
- resizePool(pool, () -> conf.getCount(p), p.getKey());
+ resizePool(pool, () -> conf.getCount(p), p.getKey() + ".pool");
Review Comment:
I'm not sure adding ".pool" is correct here. From what I can tell, this is
used for a log message and it's using the configuration property for `p`, and I
think the pool names are sufficiently divorced from the property names now.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java:
##########
@@ -262,8 +262,8 @@ private synchronized void startLogMaker() {
if (nextLogMaker != null) {
return;
}
- nextLogMaker = ThreadPools.getServerThreadPools().getPoolBuilder("WALog
creator")
- .numCoreThreads(1).enableThreadPoolMetrics().build();
+ nextLogMaker =
ThreadPools.getServerThreadPools().getPoolBuilder("tserver.walog.creator")
Review Comment:
No enum for this pool name?
##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPoolNames.java:
##########
@@ -0,0 +1,54 @@
+package org.apache.accumulo.core.util.threads;
+
+public enum ThreadPoolNames {
+
+ ACCUMULO_POOL_PREFIX("accumulo.pool"),
+ BATCH_SCANNER_POOL_NAME("accumulo.pool.client.batch.reader.scanner"),
Review Comment:
Just a suggestion, one that can be ignored. It might make sense to remove
`_NAME` from the enum name, as the proper name currently is
`ThreadPoolNames.BATCH_SCANNER_POOL_NAME`. It might be redundant.
--
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]
