smengcl commented on code in PR #11501:
URL: https://github.com/apache/ozone/pull/11501#discussion_r4300121349
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -257,6 +263,10 @@ public RpcClient(ConfigurationSource conf, String
omServiceId)
this.ecReconstructExecutor = MemoizedSupplier.valueOf(() ->
createThreadPoolExecutor(
EC_RECONSTRUCT_STRIPE_READ_POOL_MIN_SIZE,
clientConfig.getEcReconstructStripeReadPoolLimit(),
"ec-reconstruct-reader-TID-%d"));
+ // Created on first use, after ozoneManagerClient, which holds the
thread-local request context.
+ this.ecParallelReadExecutor = MemoizedSupplier.valueOf(() ->
createRequestContextPropagatingExecutor(
+ EC_PARALLEL_READ_POOL_MIN_SIZE,
clientConfig.getEcParallelReadPoolLimit(), "ec-parallel-reader-TID-%d",
Review Comment:
`EC_PARALLEL_READ_POOL_MIN_SIZE` is 3, but nothing keeps
`ozone.client.ec.parallel.read.pool.limit` at or above it. With a limit of 1 or
2, `new ThreadPoolExecutor` throws `IllegalArgumentException` (maximumPoolSize
< corePoolSize) on the first `get()`. That is unchecked, so
`ECBlockInputStreamProxy` does not fail over to reconstruction, and every
multi-cell read fails the same way.
Could the core size be `Math.min(EC_PARALLEL_READ_POOL_MIN_SIZE, limit)`, or
the limit validated? The reconstruct pool has the same gap, but this new key is
more likely to be tuned down.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -257,6 +263,10 @@ public RpcClient(ConfigurationSource conf, String
omServiceId)
this.ecReconstructExecutor = MemoizedSupplier.valueOf(() ->
createThreadPoolExecutor(
Review Comment:
The request context problem fixed here for the parallel read pool also
applies to `ecReconstructExecutor`. `ECBlockReconstructedStripeInputStream`
runs `readIntoBuffer` on these threads, through a `BlockInputStream` that can
refresh block locations from OM after a `StorageContainerException`. On an S3
Gateway with the S3 auth check on, that refresh has no S3 auth on the worker
thread and fails with `S3 Auth expected`, and it also loses the read
consistency hint.
Should this pool also be created with
`createRequestContextPropagatingExecutor`? A follow-up JIRA is fine if you
prefer to keep this PR scoped.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
