Skip to content

Conversation

@turboFei
Copy link
Member

@turboFei turboFei commented May 17, 2023

What changes were proposed in this pull request?

Adding working directory into classpath on the driver in K8S cluster mode.

Why are the changes needed?

After #37417, the spark.files, spark.jars are placed in the working directory.
But seems that the spark context classloader can not access them because they are not in the classpath by default.
This pr adds the current working directory into classpath, so that the spark.files, spark.jars placed in the working directory can be accessible by the classloader.
For example, the hive-site.xml uploaded by spark.files.

Does this PR introduce any user-facing change?

yes, users do not need to add the working directory into spark classpath manually.

How was this patch tested?

UT.

@github-actions github-actions bot added the CORE label May 17, 2023
@dongjoon-hyun
Copy link
Member

cc @pralabhkumar and @holdenk from #37417

@holdenk
Copy link
Contributor

holdenk commented May 17, 2023

+1 looks reasonable module the existing suggestions (clean up the logging + tighten the test). Thanks for making this PR :)

@pralabhkumar
Copy link
Contributor

LGTM .

@turboFei turboFei force-pushed the work_dir_classpath branch 2 times, most recently from 485a96d to 25305b4 Compare May 18, 2023 07:40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might affect other modes. A better place would be the entrypoint.sh for resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh to include work-dir into the classpath.

Copy link
Member Author

@turboFei turboFei May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we have checked the k8s cluster mode

if (isKubernetesClusterModeDriver) {
 ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't add current working dir to executor's class path right?

Just checked with yarn's behavior, yarn add CWD to both driver and executor. And it puts CWD before localized SPARK_CONF and HADOOP_CONF.

See

To get the similar behavior, I believe it would be easier to leverage the entrypoint.sh here when running on K8S.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will check it

Copy link
Member Author

@turboFei turboFei May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about now? @advancedxy
I checked the code, for driver, if just leverage the entrypoint.sh, it is difficult to keep the behavior as mentioned above.

So I just leverage the entrypoint.sh for executor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@turboFei
Copy link
Member Author

gentle ping @dongjoon-hyun @holdenk @pralabhkumar @pan3793 for the latest change, thanks a lot.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the title, not only Driver.

pan3793

This comment was marked as duplicate.

@turboFei turboFei changed the title [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver/executor in K8S cluster mode May 26, 2023
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but why this is prepended? For this specific part, I'm strong negative because this could have a side-effect like spark.executor.userClassPathFirst=true. As we know, we don't recommend userClassPathFirst at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the correction,will check and address it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but why this is prepended? For this specific part, I'm strong negative because this could have a side-effect like spark.executor.userClassPathFirst=true. As we know, we don't recommend userClassPathFirst at all.

Hi @dongjoon-hyun, just to be clear, are you against for putting the PWD into executor's class path or for the PWD being the first in the class path?

In my opinion, to align with spark on yarn's behavior, PWD should be put in both driver and executor's class path. But I'm ok for it to be last or anywhere in the class path.

By the way, this pr put PWD(.) in the first of class path for driver, if you have concern about PWD being first, the driver may have the same issue here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya. The second one. I'm only worrying about the prepending. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then, @turboFei would you mind to do some search to make sure which place the PWD should be put in the executor’s class path? sorry for the inconvenience.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late reply, I checked the code.

For driver:

image

I think for driver, now the working directory is just placed before user jars, likes

  • --jars
  • localPrimaryResource

It should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and for executor classpath, I think that we can place the working directory at the tail of the classpath.

@turboFei turboFei changed the title [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver/executor in K8S cluster mode [SPARK-43540][K8S][CORE] Add working directory into classpath on the driverin K8S cluster mode May 31, 2023
@turboFei turboFei changed the title [SPARK-43540][K8S][CORE] Add working directory into classpath on the driverin K8S cluster mode [SPARK-43540][K8S][CORE] Add working directory into classpath on the driver in K8S cluster mode May 31, 2023
@dongjoon-hyun
Copy link
Member

cc @advancedxy once more because of his #41201 (comment) . Now, the last commit reverted entrypoint.sh change .

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @turboFei and all!

@dongjoon-hyun
Copy link
Member

The sql - other tests failure is a known flaky one which is irrelevant to this one. Let me merge this PR.
Screenshot 2023-06-07 at 3 37 27 PM

@turboFei
Copy link
Member Author

turboFei commented Jun 8, 2023

thanks all!

@turboFei turboFei deleted the work_dir_classpath branch June 8, 2023 02:38
czxm pushed a commit to czxm/spark that referenced this pull request Jun 12, 2023
…driver in K8S cluster mode

### What changes were proposed in this pull request?

Adding working directory into classpath on the driver in K8S cluster mode.

### Why are the changes needed?

After apache#37417, the spark.files, spark.jars are placed  in the working directory.
But seems that the spark context classloader can not access them because they are not in the classpath by default.
This pr adds the current working directory into classpath, so that the spark.files, spark.jars placed in the working directory can be accessible by the classloader.
For example, the `hive-site.xml` uploaded by `spark.files`.

### Does this PR introduce _any_ user-facing change?

yes, users do not need to add the working directory into spark classpath manually.

### How was this patch tested?

UT.

Closes apache#41201 from turboFei/work_dir_classpath.

Authored-by: fwang12 <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Yikun added a commit to Yikun/spark-docker that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants