Repository navigation
HDDS-16867. ECBlockInputStream should read data cells from datanodes in parallel - #11501
yandrey321 wants to merge 3 commits into
Conversation
|
@jojochuang could you please take a look? |
|
@ss77892 could you please take a look? |
smengcl
left a comment
There was a problem hiding this comment.
Thanks @yandrey321 for the update
| "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", |
There was a problem hiding this 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.
| @@ -257,6 +263,10 @@ public RpcClient(ConfigurationSource conf, String omServiceId) | |||
| this.ecReconstructExecutor = MemoizedSupplier.valueOf(() -> createThreadPoolExecutor( | |||
There was a problem hiding this 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.
| } | ||
| read += cell.length; | ||
| } | ||
| position = startPosition + read; |
There was a problem hiding this comment.
Minor: when this method ends by throwing, position only moves past the cells before the first failure (and not at all when line 504 rethrows an unchecked exception). Indexes whose tasks succeeded have already moved their inner streams further, though. If seeked is still false, a later read on this stream skips the position check in readCells and seekStreamIfNecessary and reads from the wrong offset.
The proxy closes this reader on BadDataLocationException, so the main path is safe, but a caller that retries after a RuntimeException would silently get wrong data. Setting seeked = true before throwing would cover it.
| import org.apache.hadoop.ozone.OzoneConfigKeys; | ||
|
|
||
| /** Runs the S3 SDK tests with S3 Gateways that read the cells of EC keys in parallel. */ | ||
| class TestS3SDKWithEcParallelRead extends OzoneS3SDKTests { |
There was a problem hiding this comment.
This starts another cluster and reruns the whole S3 SDK suite to flip one client flag, but only the multi-cell EC GET exercises the new path. Could the parallel case run on the existing cluster instead, e.g. as a parameterized variant of the EC GET test added to AbstractS3SDKV2Tests? That would avoid roughly doubling the S3 SDK test time.
| * </pre> | ||
| */ | ||
| @Tag("benchmark") | ||
| public class TestEcParallelReadBenchmark { |
There was a problem hiding this comment.
Since this is tagged benchmark and never runs in CI, it is likely to rot. Would it be better to leave it out of the tree (or move it to a separate JIRA) and keep the results in the PR description?
Similarly, moving the audit log files under target/ looks useful but is independent of this feature, so it could be its own small change.
What changes were proposed in this pull request?
On the healthy EC read path,
ECBlockInputStreamreads one cell at a time, so a read that spansNcells costsNserial datanode round trips, even though the cells are on different datanodes. With this change, a read thatspans more than one cell reads the cells of different data indexes in parallel. The reconstruction path is already
parallel (HDDS-5952), and HDFS does the same in
StripeReader.readStripe.Key decisions:
ozone.client.ec.parallel.read.enableddefaults tofalse, and with it off,behavior is unchanged. Reads within a single cell always stay on the existing serial path.
ozone.client.ec.parallel.read.pool.limit, default 30), notthe reconstruct pool, so healthy and degraded reads do not compete for threads. The pool is created lazily.
When it is saturated, the calling thread reads the remaining cells itself.
fan-out is at most the number of data units.
every task before returning, so nothing writes into the caller's buffer after
readreturns.ECBlockInputStreamProxytreats each inner read as all-or-nothing. Every failed index of the call is reportedin one
BadDataLocationException, so spare-location retry and the failover to reconstruction each happen once.the pool.
BlockInputStreamrefreshesblock locations after a
StorageContainerExceptionor a connectivity error. The S3 authentication and theread-consistency hint that OM requests carry are thread-locals of the OM client, set on the request thread by the
S3 Gateway. The pool's
executetherefore wraps each task: it copies the submitter's values onto the worker, runsthe task, and then restores the worker's previous values, or clears them if it had none. Without this, a refresh
from a worker fails with
S3 Auth expectedwhen the S3 auth check is on, or silently uses the default readconsistency.
Out of scope: reading ahead to the end of the stripe (HDFS
curStripeBuf). Byte-array reads on encrypted (TDE)keys do not benefit, because
CryptoInputStreamreads through an 8 KB buffer, which never spans more than onecell.
Generated-by: Claude Code (Claude Opus 5.5)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16867
How was this patch tested?
with no extra configuration.
TestECBlockInputStreamandTestOzoneECClientuse@ParameterizedClass, thetwo EC tests in
TestOzoneFSInputStreamare parameterized, andTestKeyInputStreamreads through both kinds ofclient. All pass.
are in flight on all indexes at once, and it fails with
Timed out waiting for 2 concurrent readswhen theexecutor hand-off is removed. Other tests cover multi-index failure reporting and spare-location retry.
TestOzoneECClient.testLocationRefreshDuringReadKeepsRequestContextsets S3 auth and aLOCAL_LEASEhint on thereading thread. It then makes one GetBlock fail with
CONTAINER_NOT_FOUND, which forces an OM refresh, from apool thread in parallel mode. Every recorded OM request must carry both. Without the fix it fails in parallel
mode only (
GetKeyInfo sent without S3 authentication, andexpected LOCAL_LEASE but was READ_CONSISTENCY_UNSPECIFIED).TestRpcClient.testWithSubmitterContextPropagatesAndRestorescovers the wrapper: what is copied, restoring theworker's own values, clearing when it had none, and restoring when the task throws.
TestSecureOzoneRpcClient.testEcReadWithS3AuthAndReadConsistencyreads a multi-cell EC key withS3 auth and a read-consistency hint, in both modes.
AbstractS3SDKV2Testsgets an S3 GET of a multi-cellobject in an EC bucket with each read-consistency header. The new
TestS3SDKWithEcParallelReadruns the wholeS3 SDK suite with gateways that read in parallel. Both parallel cases assert that the pool was used.
TestS3SDKandTestS3SDKWithEcParallelRead: 470 run, 0 failures. These tests cannot force a worker-siderefresh (datanode restart does not work in the secure suite), so the unit test above is the regression test.
TestOzoneIntegrationNonHA(340 run, 0 failures),TestECKeyOutputStream,TestKeyInputStream.auditlog.propertiesandAuditLogTestUtilsnow write and readtarget/audit.logandtarget/system_audit.log. Before, they used the module root, and the new secure test(through
OzoneRpcClientTests, which never deletes them) left both files there.TestOmAcls(9/9) reads the newlocation.
Benchmark (
TestEcParallelReadBenchmark, taggedbenchmarkso it does not run in CI): mini cluster, 48 MBkeys, 1 MB cells, 7 rounds after warm-up, run twice. Speed-up of parallel over serial throughput:
Observations:
(about +-8%).
read()call. p99 mostly drops too, but 7rounds are too few to support tail claims.
removes are nearly free here.
than on 5, so the 5 vs 9 difference reflects CPU load, not how cells are spread over datanodes.