HADOOP-19205. S3A: initialization/close slower than with v1 SDK #6892
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
shameersss1
reviewed
Jun 19, 2024
There was a problem hiding this comment.
I really like this idea of lazy initialization of async client and the new interface for creating s3 client.
@steveloughran - Do we see any visible performance impact for this ? Let's say for all read queries where async client/transfer manager is not used ?
ahmarsuhail
requested changes
Jun 19, 2024
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Statistic.java
Outdated
Show resolved
Hide resolved
| * @throws IOException failure. | ||
| */ | ||
| private void bindAWSClient(URI name, boolean dtEnabled) throws IOException { | ||
| private ClientManager bindAWSClient(URI fsURI, boolean dtEnabled) throws IOException { |
There was a problem hiding this comment.
nit: should we rename this method to createClientManager since it's not really binding AWS clients anymore?
|
Syed, not got numbers yet, though I will try to collect some before/after values |
83ed705 to
4c9a619
Compare
4 tasks
Adds new ClientManager interface/impl which provides on-demand creation of sync and async s3 clients, s3 transfer manager, and in close() terminates these. S3A FS is modified to * Create one of these and hand off to S3Store * Use the same ClientManager interface against S3Store to demand-create the services. * only create the async client as part of the transfer manager creation, during rename. * stats on client creation count/duration are recorded. + statistics on the time to initialize and shutdown the s3afs is collected in IOStatistics for reporting. No attempt to do async creation of the s3 client in initialize, though it could offer marginal benefits, depending on the codepath. Change-Id: I79a668aacd920048447485afed77df573a38cb37
* ClientManager getOrCreate* methods are unsynchronized so one thread won't block just because the another thread is busy creating a different client * the actual creation operations are synchronized, with checks for closure inside them in case the manager is closed while they await the lock. * close() is parallelized. * some logging at debug; warn on exception during close() * some init/accessor tuning to ensure mocking tests still work... (with an unrealistic goal of making it easier...) Change-Id: I3e72172834232cc159d938c372d974d6672624f8
+ add a new FutureIO.toSupplier(CallableRaisingIOE<>) which takes a callable and returns a supplier, so its easier to schedule IOE-raising operations. + review comments Change-Id: I0a17167e3b0fdde0ff379dd5c28c945782d5dbd2
+shut up spotbugs Change-Id: Ie4cace3da846787018e6e2f7c14c6006b64dd667
shameersss1
reviewed
Jun 19, 2024
| * @return the S3 client. | ||
| * @throws IOException on any failure to create the client. | ||
| */ | ||
| private S3Client getS3Client() throws IOException { |
There was a problem hiding this comment.
is this method required when we have getOrCreateS3Client() method already ?
deec1fc to
8af6f39
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
update: going to pull out the reused code into some AtomicAutoCloseable(function) which takes a supplier as an arg and invokes it if needed, call close if not. No plans to be clever about failure handling, just retry on each get(). |
add class LazyAtomicReference<T> This take a constructor CallableRaisingIOE which is then invoked on demand to create the reference value the first time it is evaluated in get(). It has a getUnchecked() to wrap any IOEs raised, and implements CallableRaisingIOE<T> so its get() call can be chained/wrapped etc. The subclass LazyAutoCloseableReference<T extends AutoCloseable> takes AutoCloseables, implements the interface and in close() will close the inner reference if it is set. These are used in ClientManagerImpl instead of doing all the null checks &c itself. Tests for parallel creation are now more sophisticated * semaphore + sleep choreography * assertions on thread ID of thread creating the class This should be a lot less brittle to timing issues Change-Id: Idf9770c072b0436331a76f983e533528932dceff
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I92ba2afb7bae0c3c9fc0c36ecbdec0790d1c009b
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: Ia3a502f14c35022b66b0fae4a6e6326cbb145189
|
🎊 +1 overall
This message was automatically generated. |
|
tested, s3 london. One odd failure, where an s3 request was considered to be outside of a span even though the callchain implies it was within a span. not going to show up in productin where unaudited requests are allowed, and not seen on standalone test runs. Note, normally I run with 8 threads.. |
shameersss1
approved these changes
Jun 22, 2024
|
somef more changes planned in the
implementing supplier lets you something quite special with this: you can feed it in to any java api which takes a supplier, and, if the function supplied in the constructor is side-effect free & not dependent on external state, act as an on-demand cache of a function. Surprised this isn't in the JDK already, either the team aren't aware of parallel graph reduction systems, or aware enough of the 1980s designs (e.g. ALICE) that they chose to avoid. Things are changing. |
LazyAtomicReference implements Supplier as well as CallableRaisingIOE; this lets you use this as an on-demand cache of the result of an operation. with test Change-Id: Ia97acab62504b55892e91e8db70ae2bde570bbc0
|
tested with s3 london (versioned bucket), all good except for the usual No performance timings except that we know that we delay creation of async and transfer manager.
Applications which create many object store FS instances have a significant problem, independent of initialize() time: time to create a pool of https clients. Nothing we can do there except guide them into changes they need to make |
|
💔 -1 overall
This message was automatically generated. |
LazyAtomicReference: javadocs; eval() can be subclassed. LazyAutoCloseableReference: closed flag ensures close is never called twice and that eval() fails when called on a closed ref. changed LazyAtomicReference.fromSupplier() method name, added and equivalent for its AutoCloseable subclass, with a different name. Change-Id: I2936c500149be35c75ec2f617abdcd18d524f896
|
retested s3 london |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran - The changes LGTM +1. I do see some 2 seconds improvement while closing filesystem object when async client is not initialized/used. |
|
@steveloughran - Is there any more changes to be done ? I was thinking i could rebase HADOOP-18708 if this get merged. |
|
will do, i've been off all week. merged. will cherrypick. note that apart from reported close() problems related to netty threads, it'd make sense to just have one client...this patch only reduces costs if rename() isn't used. Before/while doing that, all interactions with the store should be pushed into the S3Store interface, so the rest of the code never gets direct access to S3Client |
steveloughran
added a commit
that referenced
this pull request
Jul 5, 2024
Adds new ClientManager interface/implementation which provides on-demand
creation of synchronous and asynchronous s3 clients, s3 transfer manager,
and in close() terminates these.
S3A FS is modified to
* Create a ClientManagerImpl instance and pass down to its S3Store.
* Use the same ClientManager interface against S3Store to demand-create
the services.
* Only create the async client as part of the transfer manager creation,
which will take place during the first rename() operation.
* Statistics on client creation count and duration are recorded.
+ Statistics on the time to initialize and shutdown the S3A FS are collected
in IOStatistics for reporting.
Adds to hadoop common class
LazyAtomicReference<T> implements CallableRaisingIOE<T>, Supplier<T>
and subclass
LazyAutoCloseableReference<T extends AutoCloseable>
extends LazyAtomicReference<T> implements AutoCloseable
These evaluate the Supplier<T>/CallableRaisingIOE<T> they were
constructed with on the first (successful) read of the the value.
Any exception raised during this operation will be rethrown, and on future
evaluations the same operation retried.
These classes implement the Supplier and CallableRaisingIOE
interfaces so can actually be used for to implement lazy function evaluation
as Haskell and some other functional languages do.
LazyAutoCloseableReference is AutoCloseable; its close() method will
close the inner reference if it is set
This class is used in ClientManagerImpl for the lazy S3 Cliehnt creation
and closure.
Contributed by Steve Loughran.
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
…he#6892) Adds new ClientManager interface/implementation which provides on-demand creation of synchronous and asynchronous s3 clients, s3 transfer manager, and in close() terminates these. S3A FS is modified to * Create a ClientManagerImpl instance and pass down to its S3Store. * Use the same ClientManager interface against S3Store to demand-create the services. * Only create the async client as part of the transfer manager creation, which will take place during the first rename() operation. * Statistics on client creation count and duration are recorded. + Statistics on the time to initialize and shutdown the S3A FS are collected in IOStatistics for reporting. Adds to hadoop common class LazyAtomicReference<T> implements CallableRaisingIOE<T>, Supplier<T> and subclass LazyAutoCloseableReference<T extends AutoCloseable> extends LazyAtomicReference<T> implements AutoCloseable These evaluate the Supplier<T>/CallableRaisingIOE<T> they were constructed with on the first (successful) read of the the value. Any exception raised during this operation will be rethrown, and on future evaluations the same operation retried. These classes implement the Supplier and CallableRaisingIOE interfaces so can actually be used for to implement lazy function evaluation as Haskell and some other functional languages do. LazyAutoCloseableReference is AutoCloseable; its close() method will close the inner reference if it is set This class is used in ClientManagerImpl for the lazy S3 Cliehnt creation and closure. Contributed by Steve Loughran.
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
…he#6892) Adds new ClientManager interface/implementation which provides on-demand creation of synchronous and asynchronous s3 clients, s3 transfer manager, and in close() terminates these. S3A FS is modified to * Create a ClientManagerImpl instance and pass down to its S3Store. * Use the same ClientManager interface against S3Store to demand-create the services. * Only create the async client as part of the transfer manager creation, which will take place during the first rename() operation. * Statistics on client creation count and duration are recorded. + Statistics on the time to initialize and shutdown the S3A FS are collected in IOStatistics for reporting. Adds to hadoop common class LazyAtomicReference<T> implements CallableRaisingIOE<T>, Supplier<T> and subclass LazyAutoCloseableReference<T extends AutoCloseable> extends LazyAtomicReference<T> implements AutoCloseable These evaluate the Supplier<T>/CallableRaisingIOE<T> they were constructed with on the first (successful) read of the the value. Any exception raised during this operation will be rethrown, and on future evaluations the same operation retried. These classes implement the Supplier and CallableRaisingIOE interfaces so can actually be used for to implement lazy function evaluation as Haskell and some other functional languages do. LazyAutoCloseableReference is AutoCloseable; its close() method will close the inner reference if it is set This class is used in ClientManagerImpl for the lazy S3 Cliehnt creation and closure. Contributed by Steve Loughran.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HADOOP-19205
Adds new ClientManager interface/impl which provides on-demand creation of sync and async s3 clients, s3 transfer manager, and in close() terminates these.
S3A FS is modified to
The s3client is still created in FileSystem.initialize(), it is the async one which
is on demand.
No attempt to do async creation of the s3 client in initialize, though it could offer marginal benefits, depending on the codepath.
Change-Id: I79a668aacd920048447485afed77df573a38cb37
How was this patch tested?
Relying on regression tests knowing that this codepath will be tested.
Some other tests will be needed. e.g
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?