Skip to content

[SPARK-44242] Improve Max Heap not set check #41806

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

Closed
wants to merge 3 commits into from

Conversation

ashangit
Copy link
Contributor

What changes were proposed in this pull request?

Currently if any Xmx string is available in the driver java options even if not related to Max Heap setting the job sumission failed
For Ex. this command failed
./bin/spark-submit --class org.apache.spark.examples.SparkPi --conf "spark.driver.extraJavaOptions=-Dtest=Xmx" examples/jars/spark-examples_2.12-3.4.1.jar 100

Why are the changes needed?

The command should any failed if the -Xmx argument is set not if it is part of another parameter or string

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a unitary test

@ashangit ashangit force-pushed the nfraison/SPARK-44242 branch 2 times, most recently from e072703 to 30d72e2 Compare June 30, 2023 07:26
@ashangit ashangit force-pushed the nfraison/SPARK-44242 branch 3 times, most recently from 3a19cd3 to 2f2ec2f Compare July 1, 2023 20:00
@ashangit ashangit requested review from LuciferYang and mridulm July 1, 2023 20:01
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Looks good to me.
+CC @LuciferYang

@LuciferYang
Copy link
Contributor

@ashangit Java linter check failed, please fix it when you have time

public void testCheckJavaOptionsThrowException() throws Exception {
Map<String, String> env = new HashMap<>();
List<String> sparkSubmitArgs = Arrays.asList(
parser.MASTER,
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation: should be 2 space too

public void testCheckJavaOptions() throws Exception {
Map<String, String> env = new HashMap<>();
List<String> sparkSubmitArgs = Arrays.asList(
parser.MASTER,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

LGTM, pending ci

@LuciferYang
Copy link
Contributor

@ashangit Can you rebase the code or re-trigger GA? Some testing tasks failed

ashangit added 3 commits July 4, 2023 09:19
Currently if any Xmx string is available in the driver java options
even if not related to Max Heap setting the job sumission failed
@ashangit ashangit force-pushed the nfraison/SPARK-44242 branch from 4aeea91 to 644a928 Compare July 4, 2023 07:19
@ashangit
Copy link
Contributor Author

ashangit commented Jul 4, 2023

Test seems now stuck on pyspak-sql one: https://github.com/ashangit/spark/actions/runs/5451917791/jobs/9918773466#logs without any logs after 4H
Is there a way to relaunch it?

@LuciferYang
Copy link
Contributor

You can try canceling the running GA task and then re-trigger the failed one

@ashangit ashangit requested a review from mridulm July 5, 2023 14:10
@ashangit
Copy link
Contributor Author

ashangit commented Aug 9, 2023

@mridulm can we merge this PR?

@mridulm
Copy link
Contributor

mridulm commented Aug 12, 2023

Ah, I thought @LuciferYang might have merged this one :-)

@mridulm mridulm closed this in 567823e Aug 12, 2023
@mridulm
Copy link
Contributor

mridulm commented Aug 12, 2023

Merged to master.
Thanks for fixing this @ashangit !
Thanks for the reviews @HyukjinKwon, @LuciferYang :-)

@LuciferYang
Copy link
Contributor

Sorry, I forgot about that…

@mridulm
Copy link
Contributor

mridulm commented Aug 13, 2023

Just kidding @LuciferYang :-)

hvanhovell pushed a commit to hvanhovell/spark that referenced this pull request Aug 13, 2023
### What changes were proposed in this pull request?
Currently if any Xmx string is available in the driver java options even if not related to Max Heap setting the job sumission failed
For Ex. this command failed
`./bin/spark-submit --class org.apache.spark.examples.SparkPi --conf "spark.driver.extraJavaOptions=-Dtest=Xmx"  examples/jars/spark-examples_2.12-3.4.1.jar 100`

### Why are the changes needed?
The command should any failed if the -Xmx argument is set not if it is part of another parameter or string

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

### How was this patch tested?
Added a unitary test

Closes apache#41806 from ashangit/nfraison/SPARK-44242.

Authored-by: Nicolas Fraison <nicolas.fraison@datadoghq.com>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
valentinp17 pushed a commit to valentinp17/spark that referenced this pull request Aug 24, 2023
### What changes were proposed in this pull request?
Currently if any Xmx string is available in the driver java options even if not related to Max Heap setting the job sumission failed
For Ex. this command failed
`./bin/spark-submit --class org.apache.spark.examples.SparkPi --conf "spark.driver.extraJavaOptions=-Dtest=Xmx"  examples/jars/spark-examples_2.12-3.4.1.jar 100`

### Why are the changes needed?
The command should any failed if the -Xmx argument is set not if it is part of another parameter or string

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

### How was this patch tested?
Added a unitary test

Closes apache#41806 from ashangit/nfraison/SPARK-44242.

Authored-by: Nicolas Fraison <nicolas.fraison@datadoghq.com>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
ragnarok56 pushed a commit to ragnarok56/spark that referenced this pull request Mar 2, 2024
### What changes were proposed in this pull request?
Currently if any Xmx string is available in the driver java options even if not related to Max Heap setting the job sumission failed
For Ex. this command failed
`./bin/spark-submit --class org.apache.spark.examples.SparkPi --conf "spark.driver.extraJavaOptions=-Dtest=Xmx"  examples/jars/spark-examples_2.12-3.4.1.jar 100`

### Why are the changes needed?
The command should any failed if the -Xmx argument is set not if it is part of another parameter or string

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

### How was this patch tested?
Added a unitary test

Closes apache#41806 from ashangit/nfraison/SPARK-44242.

Authored-by: Nicolas Fraison <nicolas.fraison@datadoghq.com>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
szehon-ho pushed a commit to szehon-ho/spark that referenced this pull request Aug 7, 2024
### What changes were proposed in this pull request?
Currently if any Xmx string is available in the driver java options even if not related to Max Heap setting the job sumission failed
For Ex. this command failed
`./bin/spark-submit --class org.apache.spark.examples.SparkPi --conf "spark.driver.extraJavaOptions=-Dtest=Xmx"  examples/jars/spark-examples_2.12-3.4.1.jar 100`

### Why are the changes needed?
The command should any failed if the -Xmx argument is set not if it is part of another parameter or string

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

### How was this patch tested?
Added a unitary test

Closes apache#41806 from ashangit/nfraison/SPARK-44242.

Authored-by: Nicolas Fraison <nicolas.fraison@datadoghq.com>

Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
Co-authored-by: Nicolas Fraison <nicolas.fraison@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants