HADOOP-17313. FileSystem.get to support slow-to-instantiate FS clients. #2396

steveloughran · 2020-10-19T16:38:27Z

This adds a semaphore to throttle the number of FileSystem instances which
can be created simultaneously, set in "fs.creation.parallel.count".

This is designed to reduce the impact of many threads in an application calling
FileSystem.get() on a filesystem which takes time to instantiate -for example
to an object where HTTPS connections are set up during initialization.
Many threads trying to do this may create spurious delays by conflicting
for access to synchronized blocks, when simply limiting the parallelism
diminishes the conflict, so speeds up all threads trying to access
the store.

The default value, 64, is larger than is likely to deliver any speedup -but
it does mean that there should be no adverse effects from the change.

If a service appears to be blocking on all threads initializing connections to
abfs, s3a or store, try a smaller (possibly significantly smaller) value.

Contributed by Steve Loughran.

mukund-thakur · 2020-10-21T09:21:48Z

Looks good. Just the pending test.

steveloughran · 2020-10-21T13:14:58Z

yeah, let me get that out the way before I forget about that cache. I'll use a counter of discarded instances and test off that. I'll plan some tricks to avoid the test being fussy about timing...have a semaphore inside the fake FS I'll be instantiating to block its construction, as sleep() is brittle as well as slowing down test runs

steveloughran · 2020-10-21T13:36:25Z

Also, I plan to move the close in a discard out of the locked area. It's not needed and an FS is doing anything there, we don't want to block the other threads

...p-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCaching.java

mehakmeet

LGTM. Just some small nits and checkstyles.

Have one doubt also: Is there particular reasoning for "64" as the default value?

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

...p-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCaching.java

steveloughran · 2020-10-29T17:01:07Z

./hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java:3512:      checkArgument(permits > 0 , "Invalid value of %s: %s",:33: ',' is preceded with whitespace. [NoWhitespaceBefore]
./hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java:3553:              new DurationInfo(LOGGER, false, "Acquiring creator semaphore for %s",: Line is longer than 80 characters (found 83). [LineLength]
./hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCaching.java:465:    private static final Semaphore sem = new Semaphore(1);:36: Name 'sem' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. [ConstantName]

steveloughran · 2020-10-29T17:06:19Z

Is there particular reasoning for "64" as the default value?

No. I wondered whether to make it smaller or just leave large.

Large: no visible impact of this change anywhere, so lowest risk
Small: should always speed up applications which always use it.

I think I should add a mention of this in the s3a performance doc, so it's not forgotten about

surendralilhore · 2020-11-02T06:04:54Z

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

What if close() throw IOException ?
Even it has cached object, it will not return that.

That is an interesting thought. I just left that code as is, but yes, it could fail

Looking at FileSystem.close, the removal of the entry from the cache should be in a finally clause too, shouldn't it. ouch. Ignoring that for now.

How about I go to IOUtils.close() & catch and log on failures.

Looking at FileSystem.close, the removal of the entry from the cache should be in a finally clause too, shouldn't it. ouch. Ignoring that for now.

I feel not required, processDeleteOnExit() already catching the IOException.

How about I go to IOUtils.close() & catch and log on failures.

I didn't get this, are you planning to use IOUtils.close() to close filesystem instance ?

I feel not required, processDeleteOnExit() already catching the IOException.

yes, but look at the subclasses

steveloughran · 2020-11-19T14:52:49Z

Don't get the test failure. Will rebase

[ERROR] testServerKeyPasswordDefaultsToPassword(org.apache.hadoop.security.ssl.TestSSLFactory)  Time elapsed: 0.103 s  <<< ERROR!
java.security.GeneralSecurityException: The property 'ssl.server.keystore.location' has not been set in the ssl configuration file.
	at org.apache.hadoop.security.ssl.FileBasedKeyStoresFactory.init(FileBasedKeyStoresFactory.java:153)
	at org.apache.hadoop.security.ssl.SSLFactory.init(SSLFactory.java:187)
	at org.apache.hadoop.security.ssl.TestSSLFactory.checkSSLFactoryInitWithPasswords(TestSSLFactory.java:520)
	at org.apache.hadoop.security.ssl.TestSSLFactory.checkSSLFactoryInitWithPasswords(TestSSLFactory.java:440)
	at org.apache.hadoop.security.ssl.TestSSLFactory.testServerKeyPasswordDefaultsToPassword(TestSSLFactory.java:397)

Adds a semaphore to throttle the #of parallel clients which can be created simultaneously, set in "fs.creation.parallel.count". Change-Id: I794cecac4a23ae7e1aa376e16b4085ea5ae20086

* Move FileSystem.close() outside the semaphored block, so it does not hold up other threads. * Tests which use the count of discarded instances for their assertions. Change-Id: I4dfaf6f2a327142048438b1bfa4c4517e595df50

* Address Mehakmeet's comments * Checkstyles * And document in s3 performance file Change-Id: I9568f980f4e9917448a1d4d1c7c2d070a6f28bad

if, in FileSystem.get(), a superfluous instance is closed(), exceptions there are swallowed. Also: put the deregistration of the class from the cache into a finally block, so even if a subclass's processDeleteOnExit code raises any exception, the filesystem is removed from the cache Change-Id: I513215fe266bbad7a864481d0505cc9073a7c35a

steveloughran · 2020-11-24T13:16:52Z

@surendralilhore -could you look @ this again? Think I've tried to address your comments

surendralilhore · 2020-11-24T15:03:46Z

@surendralilhore -could you look @ this again? Think I've tried to address your

+1

steveloughran · 2020-11-25T14:23:46Z

thanks -merging to 3.3+!

…s. (#2396) This adds a semaphore to throttle the number of FileSystem instances which can be created simultaneously, set in "fs.creation.parallel.count". This is designed to reduce the impact of many threads in an application calling FileSystem.get() on a filesystem which takes time to instantiate -for example to an object where HTTPS connections are set up during initialization. Many threads trying to do this may create spurious delays by conflicting for access to synchronized blocks, when simply limiting the parallelism diminishes the conflict, so speeds up all threads trying to access the store. The default value, 64, is larger than is likely to deliver any speedup -but it does mean that there should be no adverse effects from the change. If a service appears to be blocking on all threads initializing connections to abfs, s3a or store, try a smaller (possibly significantly smaller) value. Contributed by Steve Loughran. Change-Id: I57161b026f28349e339dc8b9d74f6567a62ce196

…te FS clients. (apache#2396) This adds a semaphore to throttle the number of FileSystem instances which can be created simultaneously, set in "fs.creation.parallel.count". This is designed to reduce the impact of many threads in an application calling FileSystem.get() on a filesystem which takes time to instantiate -for example to an object where HTTPS connections are set up during initialization. Many threads trying to do this may create spurious delays by conflicting for access to synchronized blocks, when simply limiting the parallelism diminishes the conflict, so speeds up all threads trying to access the store. The default value, 64, is larger than is likely to deliver any speedup -but it does mean that there should be no adverse effects from the change. If a service appears to be blocking on all threads initializing connections to abfs, s3a or store, try a smaller (possibly significantly smaller) value. Contributed by Steve Loughran. Change-Id: I57161b026f28349e339dc8b9d74f6567a62ce196

steveloughran added enhancement fs labels Oct 20, 2020

apache deleted a comment from hadoop-yetus Oct 26, 2020

mehakmeet reviewed Oct 28, 2020

View reviewed changes

...p-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCaching.java Outdated Show resolved Hide resolved

mehakmeet reviewed Oct 29, 2020

View reviewed changes

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java Outdated Show resolved Hide resolved

...p-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileSystemCaching.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

surendralilhore reviewed Nov 5, 2020

View reviewed changes

apache deleted a comment from hadoop-yetus Nov 9, 2020

steveloughran force-pushed the s3/HADOOP-17313-fs-get-semaphore branch from fa4b65d to 40f9e98 Compare November 17, 2020 15:20

This comment has been minimized.

Sign in to view

steveloughran force-pushed the s3/HADOOP-17313-fs-get-semaphore branch from 40f9e98 to 5c41240 Compare November 19, 2020 14:59

This comment has been minimized.

Sign in to view

steveloughran added 4 commits November 23, 2020 15:52

HADOOP-17313. FileSystem.get to support slow-to-instantiate FS clients.

ae76954

Adds a semaphore to throttle the #of parallel clients which can be created simultaneously, set in "fs.creation.parallel.count". Change-Id: I794cecac4a23ae7e1aa376e16b4085ea5ae20086

HADOOP-17313. Semaphored FS instantiation.

b64a1b3

* Move FileSystem.close() outside the semaphored block, so it does not hold up other threads. * Tests which use the count of discarded instances for their assertions. Change-Id: I4dfaf6f2a327142048438b1bfa4c4517e595df50

HADOOP-17313: FileSystem caching.

8e91d96

* Address Mehakmeet's comments * Checkstyles * And document in s3 performance file Change-Id: I9568f980f4e9917448a1d4d1c7c2d070a6f28bad

steveloughran force-pushed the s3/HADOOP-17313-fs-get-semaphore branch from 5c41240 to 788a73b Compare November 23, 2020 15:52

This comment has been minimized.

Sign in to view

steveloughran merged commit ac7045b into apache:trunk Nov 25, 2020

steveloughran deleted the s3/HADOOP-17313-fs-get-semaphore branch October 15, 2021 19:50

imjalpreet mentioned this pull request May 19, 2025

Upgrade to Hadoop 3.4.1 and JDK 17 prestodb/presto-hadoop-apache#68

Closed

HADOOP-17313. FileSystem.get to support slow-to-instantiate FS clients. #2396

HADOOP-17313. FileSystem.get to support slow-to-instantiate FS clients. #2396

Uh oh!

Conversation

steveloughran commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mukund-thakur commented Oct 21, 2020

Uh oh!

steveloughran commented Oct 21, 2020

Uh oh!

steveloughran commented Oct 21, 2020

Uh oh!

Uh oh!

mehakmeet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveloughran commented Oct 29, 2020

Uh oh!

steveloughran commented Oct 29, 2020

Uh oh!

This comment has been minimized.

surendralilhore Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

steveloughran Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

surendralilhore Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

steveloughran Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

steveloughran commented Nov 19, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

steveloughran commented Nov 24, 2020

Uh oh!

surendralilhore commented Nov 24, 2020

Uh oh!

steveloughran commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

steveloughran commented Oct 19, 2020 •

edited

Loading

mehakmeet left a comment •

edited

Loading