Skip to content

Conversation

@andrusha
Copy link
Contributor

@andrusha andrusha commented Feb 16, 2018

For some JVM options, like -XX:+UnlockExperimentalVMOptions ordering is necessary.

What changes were proposed in this pull request?

Keep original extraJavaOptions ordering, when passing them through environment variables inside the Docker container.

How was this patch tested?

Ran base branch a couple of times and checked startup command in logs. Ordering differed every time. Added sorting, ordering was consistent to what user had in extraJavaOptions.

For some JVM options, like `-XX:+UnlockExperimentalVMOptions` ordering is necessary.
@andrusha andrusha changed the title Preserve extraJavaOptions ordering [SPARK-23449] Preserve extraJavaOptions ordering Feb 16, 2018
@andrusha andrusha changed the title [SPARK-23449] Preserve extraJavaOptions ordering [SPARK-23449][K8S] Preserve extraJavaOptions ordering Feb 16, 2018
Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiangxb1987
Copy link
Contributor

also cc @vanzin to verify this change.

@vanzin
Copy link
Contributor

vanzin commented Feb 26, 2018

ok to test

@SparkQA
Copy link

SparkQA commented Feb 26, 2018

Test build #87680 has finished for PR 20628 at commit 6759e9e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Feb 26, 2018

LGTM, pending tests. I hope we can just use spark-submit here at some point.

@vanzin
Copy link
Contributor

vanzin commented Feb 26, 2018

Merging to master / 2.3.

asfgit pushed a commit that referenced this pull request Feb 26, 2018
For some JVM options, like `-XX:+UnlockExperimentalVMOptions` ordering is necessary.

## What changes were proposed in this pull request?

Keep original `extraJavaOptions` ordering, when passing them through environment variables inside the Docker container.

## How was this patch tested?

Ran base branch a couple of times and checked startup command in logs. Ordering differed every time. Added sorting, ordering was consistent to what user had in `extraJavaOptions`.

Author: Andrew Korzhuev <[email protected]>

Closes #20628 from andrusha/patch-2.

(cherry picked from commit 185f5bc)
Signed-off-by: Marcelo Vanzin <[email protected]>
@asfgit asfgit closed this in 185f5bc Feb 26, 2018
@andrusha andrusha deleted the patch-2 branch March 13, 2018 14:24
andrusha added a commit to andrusha/spark that referenced this pull request Apr 5, 2018
For some JVM options, like `-XX:+UnlockExperimentalVMOptions` ordering is necessary.

## What changes were proposed in this pull request?

Keep original `extraJavaOptions` ordering, when passing them through environment variables inside the Docker container.

## How was this patch tested?

Ran base branch a couple of times and checked startup command in logs. Ordering differed every time. Added sorting, ordering was consistent to what user had in `extraJavaOptions`.

Author: Andrew Korzhuev <[email protected]>

Closes apache#20628 from andrusha/patch-2.

(cherry picked from commit 185f5bc)
Signed-off-by: Marcelo Vanzin <[email protected]>
peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
For some JVM options, like `-XX:+UnlockExperimentalVMOptions` ordering is necessary.

## What changes were proposed in this pull request?

Keep original `extraJavaOptions` ordering, when passing them through environment variables inside the Docker container.

## How was this patch tested?

Ran base branch a couple of times and checked startup command in logs. Ordering differed every time. Added sorting, ordering was consistent to what user had in `extraJavaOptions`.

Author: Andrew Korzhuev <[email protected]>

Closes apache#20628 from andrusha/patch-2.

(cherry picked from commit 185f5bc)
Signed-off-by: Marcelo Vanzin <[email protected]>
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