-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-2849] bin/spark-submit should respect spark.driver.* for client mode #1770
Conversation
Hmmm... what does "respect" mean here though? In client mode, the vm running SparkSubmit is the same one running the driver, so it's too late to do anything with |
QA tests have started for PR 1770. This patch merges cleanly. |
Good point... well, for |
Oops, looks like However, the open issue is that if the user specifies |
QA results for PR 1770: |
QA tests have started for PR 1770. This patch merges cleanly. |
QA tests have started for PR 1770. This patch merges cleanly. |
QA results for PR 1770: |
test this please |
QA tests have started for PR 1770. This patch merges cleanly. |
QA results for PR 1770: |
QA results for PR 1770: |
test this please |
QA tests have started for PR 1770. This patch merges cleanly. |
QA results for PR 1770: |
test this please |
Before my latest set of commits we were not parsing quoted arguments (with whitespaces, quotes and backslashes and everything) properly. This is now fixed as of the latest changes, and I have included an example of what we expect from |
QA tests have started for PR 1770. This patch merges cleanly. |
One proposal - maybe we can just say in the docs that explain |
QA results for PR 1770: |
Well, we technically do support multi-line option, except for the very specific case of By the way, please don't merge this yet. I still need to verify whether the existing way of parsing these extra java options in scala is not affected. |
The previous code used to ignore all closing quotes if the same token also has an escaped double quote. For example, in -Dkey="I am the \"man\"" the last token contains both escaped quotes and valid quotes. This used to be interpreted as a token that doesn't have a closing quote when it actually does. This is fixed in this commit.
QA tests have started for PR 1770. This patch merges cleanly. |
This is so that the way this is parsed and the way Java parses its java opts is consistent.
QA results for PR 1770: |
We previously never dealt with this correctly, in that we evaluated all backslashes twice, once when passing spark.*.extraJavaOptions into SparkSubmit, and another time when calling Utils.splitCommandString. This means we need to pass the raw values of these configs directly to the JVM without evaluating the backslashes when launching SparkSubmit. The way we do this is through a few custom environment variables. As of this commit, the user should follow the format outlined in spark-defaults.conf.template for spark.*.extraJavaOptions, and the expected java options (with quotes, whitespaces and backslashes and everything) will be propagated to the driver or the executors correctly.
... to highlight our new-found ability to deal with special values.
QA tests have started for PR 1770. This patch merges cleanly. |
QA results for PR 1770: |
The size of this PR is growing beyond the original expectation. I will close this and open another one that describes the issue the new changes are solving more precisely. |
QA tests have started for PR 1770. This patch merges cleanly. |
Closing this in favor of #1845 |
QA results for PR 1770: |
### What changes were proposed in this pull request? The pr aims to upgrade FasterXML jackson from 2.15.2 to 2.16.0. ### Why are the changes needed? New version that fix some bugs, release notes as follows: - 2.1.6.0 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.16, eg: [Databind](https://github.com/FasterXML/jackson-databind) [#1770](FasterXML/jackson-databind#1770): Incorrect deserialization for BigDecimal numbers - 2.15.3 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15.3, eg: [Databind](https://github.com/FasterXML/jackson-databind) [#3968](FasterXML/jackson-databind#3968): Records with additional constructors failed to deserialize The last upgrade occurred 6 months ago, #41414 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #43859 from panbingkun/SPARK-45967. Authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
We currently respect these configs only for cluster mode. From the names of the configs themselves, however, there is every reason for the user to believe that these should also apply for client mode. This currently does not work only because we're missing the code for it, and this PR adds this missing code.