[FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs #23164

architgyl · 2023-08-08T05:36:07Z

What is the purpose of the change

This change aims to enhance the handling of classpath configuration when using the yarn.provided.lib.dirs property in a YARN environment. Currently, the yarn.ship-files property is used to add specific files to the classpath by including their parent directories. This is particularly useful for cases where resources like hive-site.xml need to be accessible to the application. In the specific context of this situation, an issue arises due to the fact that HiveConf attempts to load hive-site.xml using Thread.currentThread().getContextClassLoader().getResource("hive-site.xml"), which isn't feasible because the parent directory of hive-site.xml is not included within Flink's classpath.

The proposed change is to extend this approach to include the parent directory of files specified in yarn.provided.lib.dirs to the classpath and not the resources itself other than jar files. This will ensure that resources in these directories are available for the application's classpath enabling access to required configuration files.

Gist reference to show classpath before and after code change : Link

Brief change log

Adding parent directory of all the resources given in the yarn.provided.lib.dirsto the classpath and not adding resources itself except jar files.

Verifying this change

Verified the change by adding hive-site.xml file full path to the classpath and was able to submit Flink job successfully and interact with Hive.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? not applicable

flinkbot · 2023-08-08T05:40:24Z

CI report:

f09a4e6 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

venkata91 · 2023-08-09T18:31:22Z

@architgyl Could you please clarify the description a bit? It is not quite clear why it doesn't for hive-site.xml alone.

architgyl · 2023-08-09T19:04:19Z

@architgyl Could you please clarify the description a bit? It is not quite clear why it doesn't for hive-site.xml alone.

Updated the description.

venkata91 · 2023-08-09T21:17:16Z

@architgyl Could you please clarify the description a bit? It is not quite clear why it doesn't for hive-site.xml alone.

Updated the description.

In this case, this is an issue because HiveConf tries to load hive-site.xml using Thread.currentThread().getContextClassLoader().getResource("hive-site.xml") right which isn't visible because the parent dir of the hive-site.xml is not in Flink's classpath

architgyl · 2023-08-10T17:53:26Z

@becketqin @wangyang0918 can you please help review this PR.

venkata91 · 2023-08-10T18:10:49Z

Could you please add the before and after classpath output? Please add it to description if it is small if not a gist probably would be better.

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java

flink-yarn/src/test/java/org/apache/flink/yarn/YarnApplicationFileUploaderTest.java

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java

becketqin

@architgyl Thanks for the patch. I had a comment regarding how to make the resource files more explicit.

Also ping @wangyang0918 for review.

becketqin · 2023-08-15T13:49:39Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java

+                        URI parentDirectoryUri = new Path(fileName).getParent().toUri();
+                        String relativeParentDirectory =
+                                new Path(filePath.getName())
+                                        .toUri()
+                                        .relativize(parentDirectoryUri)
+                                        .toString();
+
+                        if (!resourcesDir.contains(relativeParentDirectory)) {
+                            resourcesDir.add(relativeParentDirectory);
+                        }
+                        resources.add(fileName);


I am wondering if naively adding all the files that are not Flink dist-jar or plugin as a resource file is over killing. One potential solution more accurate / explicit is to do something similar to the plugins. i.e. introduce a reserved keyword of "resources" as a dir name. All the files in this dir will be treated as resources.

BTW, given that this patch introduces a user sensible behavior change, we need a FLIP.

The existing config yarn.provided.lib.dirs supports adding all the resources (jars or configs) under each of those dirs separated by comma in to the classpath. Why adding the parent dir also to the classpath is an overkill?

Take yarn.ship-files as an example takes arbitrary list of dirs and adds both the files and the dir under each of the individual dirs to the classpath. Isn't this the case with yarn.provided.lib.dirs as well? Except that in the yarn.provided.lib.dirs is more of a platform specific libs which won't change between different flink apps.

Also introducing a new resources dir and only supporting that feels limiting. Ideally, users or platforms would like to logically group resources in to their own dirs. For eg: hadoop, hive libs etc in to their own dirs.

The main concern is the length of class path. I think there is a limit of 32K characters. For one of my simple local job, the class path is already over 10K chars. If we are including all the parent dir into the class path. This might easily go beyond the limit. Also, in general, adding the resource dir to class path in a more explicit and accurate way should probably be preferred.

Also introducing a new resources dir and only supporting that feels limiting. Ideally, users or platforms would like to logically group resources in to their own dirs. For eg: hadoop, hive libs etc in to their own dirs.

Maybe we can have a yarn.provided.resources.dirs in this case.

One thing to add here is, we are already relativizing the path so there are no redundant top level directory names added to the classpath. For eg: /users/test/lib/hadoop/;/users/test/lib/hive - .:hadoop:hive: will be added to the classpath.

Note: the same applies to existing yarn.ship-files config as well which is working fine without issues.

Also, in general, adding the resource dir to class path in a more explicit and accurate way should probably be preferred.

Do you mean resources as anything other than jar like .xml, .yaml etc should be added through yarn.provided.resources.dirs? Can you please explain what do you mean when you say more explicit and accurate way?

Given this separation, won't the same issue still happen for *.jar?

This reverts commit d92aa4b.

architgyl · 2023-08-21T16:47:53Z

@GJL can you please help review this PR.

wangyang0918

Sorry for the late response due to some internal things and vacation.

I left a comment that we might need some discussion.

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java

venkata91

LGTM overall, except for some minor nits in test.

flink-yarn/src/test/java/org/apache/flink/yarn/YarnApplicationFileUploaderTest.java

wangyang0918

Thanks for addressing the comments. LGTM.

+1 for merging

wangyang0918 · 2023-09-13T10:01:02Z

@architgyl Could you please verify this PR in a real YARN cluster whether it solves your original requirement about hive config? After then I will merge this PR.

architgyl · 2023-09-13T19:39:22Z

@architgyl Could you please verify this PR in a real YARN cluster whether it solves your original requirement about hive config? After then I will merge this PR.

I have verified the changes and it works as expected.

Add parent dir classpath

3676411

architgyl changed the title ~~Add parent dir classpath~~ [FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs Aug 8, 2023

architgyl and others added 2 commits August 7, 2023 22:53

Merge branch 'apache:master' into FLINK-32775

f59ddf5

Add unit test for testing parent directory addition to the classpath

ae920ec

venkata91 suggested changes Aug 10, 2023

View reviewed changes

Code changes to add resource directories first and then resource file

b2454b3

venkata91 suggested changes Aug 11, 2023

View reviewed changes

flink-yarn/src/test/java/org/apache/flink/yarn/YarnApplicationFileUploaderTest.java Show resolved Hide resolved

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java Outdated Show resolved Hide resolved

Update test to manually contruct classPath and update comment

5c8a145

becketqin reviewed Aug 15, 2023

View reviewed changes

architgyl added 2 commits August 15, 2023 13:28

Special handle resource files like plugins and update test case

d92aa4b

Revert "Special handle resource files like plugins and update test case"

039fc44

This reverts commit d92aa4b.

wangyang0918 reviewed Sep 7, 2023

View reviewed changes

flink-yarn/src/main/java/org/apache/flink/yarn/YarnApplicationFileUploader.java Outdated Show resolved Hide resolved

Update code to only add parent dir and not resources

8ad2d9d

venkata91 suggested changes Sep 12, 2023

View reviewed changes

Update test case to have code inline

f09a4e6

wangyang0918 approved these changes Sep 13, 2023

View reviewed changes

venkata91 approved these changes Sep 13, 2023

View reviewed changes

wangyang0918 merged commit 1f96218 into apache:master Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs #23164

[FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs #23164

architgyl commented Aug 8, 2023 •

edited

flinkbot commented Aug 8, 2023 •

edited

venkata91 commented Aug 9, 2023

architgyl commented Aug 9, 2023

venkata91 commented Aug 9, 2023 •

edited

architgyl commented Aug 10, 2023

venkata91 commented Aug 10, 2023

becketqin left a comment

becketqin Aug 15, 2023

venkata91 Aug 16, 2023

becketqin Aug 17, 2023

venkata91 Aug 17, 2023 •

edited

architgyl commented Aug 21, 2023

wangyang0918 left a comment

venkata91 left a comment

wangyang0918 left a comment

wangyang0918 commented Sep 13, 2023 •

edited

architgyl commented Sep 13, 2023 •

edited

[FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs #23164

[FLINK- 32775]Add parent dir of files to classpath using yarn.provided.lib.dirs #23164

Conversation

architgyl commented Aug 8, 2023 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Aug 8, 2023 • edited

CI report:

venkata91 commented Aug 9, 2023

architgyl commented Aug 9, 2023

venkata91 commented Aug 9, 2023 • edited

architgyl commented Aug 10, 2023

venkata91 commented Aug 10, 2023

becketqin left a comment

Choose a reason for hiding this comment

becketqin Aug 15, 2023

Choose a reason for hiding this comment

venkata91 Aug 16, 2023

Choose a reason for hiding this comment

becketqin Aug 17, 2023

Choose a reason for hiding this comment

venkata91 Aug 17, 2023 • edited

Choose a reason for hiding this comment

architgyl commented Aug 21, 2023

wangyang0918 left a comment

Choose a reason for hiding this comment

venkata91 left a comment

Choose a reason for hiding this comment

wangyang0918 left a comment

Choose a reason for hiding this comment

wangyang0918 commented Sep 13, 2023 • edited

architgyl commented Sep 13, 2023 • edited

architgyl commented Aug 8, 2023 •

edited

flinkbot commented Aug 8, 2023 •

edited

venkata91 commented Aug 9, 2023 •

edited

venkata91 Aug 17, 2023 •

edited

wangyang0918 commented Sep 13, 2023 •

edited

architgyl commented Sep 13, 2023 •

edited