-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-38302][K8S][TESTS] Use Java 17
in K8S IT in case of spark-tgz
option
#35627
Conversation
cc @dongjoon-hyun @sarutak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pinging me, @dcoliversun .
Can one of the admins verify this patch? |
Java 17
in K8S IT in case of spark-tgz
option
@@ -106,7 +106,7 @@ then | |||
# OpenJDK base-image tag (e.g. 8-jre-slim, 11-jre-slim) | |||
JAVA_IMAGE_TAG_BUILD_ARG="-b java_image_tag=$JAVA_IMAGE_TAG" | |||
else | |||
JAVA_IMAGE_TAG_BUILD_ARG="-f $DOCKER_FILE" | |||
JAVA_IMAGE_TAG_BUILD_ARG="-f $DOCKER_FILE_BASE_PATH$DOCKER_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to break the build when we use a docker file as an argument. For example, what happens when we give an absolute path? Please try this patch with --docker-file
option from your environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I handle this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. These code can check that $DOCKER_FILE
is absolute path or not.
- If
$DOCKER_FILE
is absolute path, nothing changes. - If relative path,
$DOCKER_FILE_BASE_PATH
and$DOCKER_FILE
are joined together, for using Java 17 in K8S IT by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that new commit do that, @dcoliversun ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. First commit doesn't supoort --docker-file
is absolute path.
Also, cc @LuciferYang since this is Java 17. |
Is the behavior of Java 17 and Java 8 inconsistent ? |
Hi @LuciferYang . Not compatibility issue, it's about a relative path about Dockerfile.java17 is not available in |
resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
Show resolved
Hide resolved
@dongjoon-hyun @dcoliversun Sorry for the late reply. This is the first time I run the test locally of this module, so I spent some time setup the local test environment. I use following command to build a tarball use
then checkout this pr use command:
run test use command as follows:
when $TRBALL_TO_TEST use absolute path, this pr can fix the described issue |
Thanks for your verification. I'm glad this pr can fix the issue😀 |
Thank you so much again, @LuciferYang ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. I also verified this PR manually. Thank you, @dcoliversun , @LuciferYang , and @martin-g . Merged to master for Apache Spark 3.3.0.
What changes were proposed in this pull request?
This PR aims to use Java 17 in K8s integration tests by default when setting spark-tgz.
Why are the changes needed?
When setting parameters
spark-tgz
during integration tests, the error thatresource-managers/kubernetes/docker/src/main/dockerfiles/spark/Dockerfile.java17
cannot be found occurs.This is due to the default value of
spark.kubernetes.test.dockerFile
is a relative path.When using the tgz, the working directory is
$UNPACKED_SPARK_TGZ
, and the relative path is invalid.Does this PR introduce any user-facing change?
No
How was this patch tested?
Runing k8s integration test manaully:
sbt
maven with spark-tgz
maven without spark-tgz