TEZ-4479: Eagerly Init/Load FileSystem In Tez Task Containers #274
+40
−0
Conversation
This file contains 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. |
|
@abstractdog Could you please review the changes? |
abstractdog
reviewed
Mar 25, 2023
| * String value. Comma seperated list of FileSystem paths which needs to be eagerly initialized. | ||
| * For example s3://bucket/,file://,hdfs://localhost:8020/ | ||
| */ | ||
| @ConfigurationScope(Scope.AM) |
There was a problem hiding this comment.
- Scope is VERTEX as we're doing this in TezChild
- constant name should contain TEZ_ prefix
abstractdog
reviewed
Mar 25, 2023
| public void run() { | ||
| try { | ||
| new Path(path).getFileSystem(conf); | ||
| LOG.info("Eagerly initiated FileSystem at path {}", path); |
There was a problem hiding this comment.
it would be nice to measure the time spent with initialization and print it to logs too
abstractdog
reviewed
Mar 25, 2023
| if (eagerInitFsPool == null && !eagerInitPaths.isEmpty()) { | ||
| eagerInitFsPool = Executors.newCachedThreadPool(new ThreadFactoryBuilder() | ||
| .setDaemon(true) | ||
| .setPriority(Thread.MAX_PRIORITY) |
There was a problem hiding this comment.
in the code, I can see we tend not to set priority in thread pools...I guess we can remove this to simplify this code further
abstractdog
reviewed
Mar 25, 2023
| @Override | ||
| public void run() { | ||
| try { | ||
| new Path(path).getFileSystem(conf); |
There was a problem hiding this comment.
shouldn't this filesystem be closed? does it hold any resources when it's open?
There was a problem hiding this comment.
Wouldn't this be gc'ed eventually? I can see any instances in the code were we are not explicitly closing the filesystem object. For example : https://github.com/apache/tez/blob/master/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java#L136
|
💔 -1 overall
This message was automatically generated. |
rbalamohan
reviewed
Mar 27, 2023
| @@ -503,6 +506,33 @@ public static TezChild newTezChild(Configuration conf, String host, int port, St | |||
| hadoopShim); | |||
| } | |||
|
|
|||
| private static void eagerInitFileSystemPaths(Configuration conf) { | |||
There was a problem hiding this comment.
It will be good to measure the time spent on FS init (even for cloudstores) and share the details, before trying out this patch.
Reason: This get inited in TezChild, but for running container, it closes the FileSystem explicitly via "FileSystem.closeAllForUGI(childUGI);". Refer "public ContainerExecutionResult run()" method.
Even if this gets early inited, it will not have major impact in container reuse scenario. It will be good to measure and find out the timing spent in FS init.
| .build()); | ||
| } | ||
| for (String path : eagerInitPaths) { | ||
| eagerInitFsPool.execute(new Runnable() { |
There was a problem hiding this comment.
before rushing to create lots of fs instances in parallel, look at HADOOP-17313 and why we actually added semaphores to stop apps like tez creating too many at the same time. this code may cause overload problems, or the fs semaphore will hold you back for safety.
best to look at why its taking so long; if s3a bucket existence checks aren't involved, then it'll be whatever auth mechanism is plugged in. same for abfs
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.
Initing/Loading FileSystem such as S3 can take ~10s - ~20s when called for the first time and the time taken for subsequent calls are negligable. If we can load the FileSystem much before it is used can help us to save some time. It can be especially useful in case of pre-warm Tez containers where the Tez task containers comes up when the Application Master (AM) is launched and not on-demand which is the default behavior. It can be also useful in cases where the Mapper tasks spends considerable time consuming the upstream shuffle data and then heads to process some FileSystem operations, in all such cases we have few FileSystem load up time.