Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-7410][CORE] Add test args to run spark-ut and use JAVA_HOME java to build thirdparty lib #7411

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[GLUTEN-7410][CORE] Add test args to run spark-ut and use JAVA_HOME java to build thirdparty lib #7411

wants to merge 5 commits into from
+28 −1

Conversation

CodenameGHOST007
Copy link

What changes were proposed in this pull request?

Spark UTs need the above args to run and had to be supplied seperately while running the tests. Third party lib was build using the jar on PATH of system. Instead it should use the JAVA_HOME one.

(Fixes: GLUTEN-7410)

How was this patch tested?

Ran Spark UTs

@github-actions github-actions bot added CORE works for Gluten Core BUILD labels Oct 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024

#7410

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024

Run Gluten Clickhouse CI

@surnaik
Copy link
Contributor

surnaik commented Oct 4, 2024

@zhouyuan PTAL, thanks!

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024

Run Gluten Clickhouse CI

@surnaik
Copy link
Contributor

surnaik commented Oct 4, 2024

@CodenameGHOST007 We do have github CI that runs for Java 8, 11 and 17, the tests run fine without this extra arguments

@surnaik
Copy link
Contributor

surnaik commented Oct 4, 2024

Can you maybe check once in the previous CI runs to see if all the tests are running fine or not. Thanks!

@surnaik
Copy link
Contributor

surnaik commented Oct 4, 2024

Run Gluten Clickhouse CI

@CodenameGHOST007
Copy link
Author

I checked the workflows that run spark-uts for velox backend in velox_backend.yml . All of them just use jdk 8 as the base where this is not a problem. I couldn't find any jdk 17 env that is running spark-uts. There are though jdk 17 envs that are just used to build and run tpc-h etc.

@surnaik
Copy link
Contributor

surnaik commented Oct 4, 2024

LGTM (Non Binding).

Would ideally love to also have maybe a JDK 17 CI, but I know we're trying to reduce the overall CI runtime and cost, maybe something Yuan or @FelixYBW can decide. Thanks!

zhztheplayer
zhztheplayer reviewed Oct 4, 2024
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024

Run Gluten Clickhouse CI

@CodenameGHOST007
Copy link
Author

Looks like the ClickHouse CI failed with

[2024-10-04T13:15:16.346Z] GlutenParquetProtobufCompatibilitySuite:
[2024-10-04T13:15:16.346Z] - unannotated array of primitive type *** FAILED ***
[2024-10-04T13:15:16.346Z]   spark.test.home or SPARK_HOME is not set. (SparkFunSuite.scala:128)
[2024-10-04T13:15:16.346Z] - unannotated array of struct *** FAILED ***
[2024-10-04T13:15:16.346Z]   spark.test.home or SPARK_HOME is not set. (SparkFunSuite.scala:128)
[2024-10-04T13:15:16.346Z] - struct with unannotated array !!! IGNORED !!!
[2024-10-04T13:15:16.346Z] - unannotated array of struct with unannotated array *** FAILED ***
[2024-10-04T13:15:16.346Z]   spark.test.home or SPARK_HOME is not set. (SparkFunSuite.scala:128)
[2024-10-04T13:15:16.346Z] - unannotated array of string *** FAILED ***
[2024-10-04T13:15:16.346Z]   spark.test.home or SPARK_HOME is not set. (SparkFunSuite.scala:128)

SPARK_HOME not set.

@CodenameGHOST007
Copy link
Author

Run Gluten Clickhouse CI

@CodenameGHOST007
Copy link
Author

CodenameGHOST007 commented Oct 7, 2024 •
edited
Loading

It seems to be because of -DargLine="-Dspark.test.home=/tmp/spark32" set in jenkins CI. Ideally it should be overriding the value set in pom. Would need some help here.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024

Run Gluten Clickhouse CI

zhztheplayer
zhztheplayer reviewed Oct 8, 2024
View reviewed changes
dev/package.sh Outdated
Comment on lines 11 to 14
if [[ -n "$JAVA_HOME" ]]; then
echo "JAVA_HOME needs to be set for Gluten Compilation"
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

s/Compilation/compilation

What's the major consideration to make JAVA_HOME required?

Copy link
Author

Choose a reason for hiding this comment

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

General best practice and mainly to avoid inconsistency since if the system has JAVA_HOME set gluten compilation will happen on that but third party libs will be generated with the Java on PATH of the system.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

Run Gluten Clickhouse CI

@CodenameGHOST007
[GLUTEN-7410][CORE] Add test args to run spark-ut and use JAVA_HOME j…
f065205 
…ava to build thirdparty lib

Spark UTs need the above args to run and had to be supplied seperately
while running the tests. Third party lib was build using the jar on PATH
of system. Instead it should use the JAVA_HOME one.
@CodenameGHOST007
Exit if java_home is not set
2232260
@CodenameGHOST007
Correct the regex to only filter out files with name core
be5d737
@CodenameGHOST007
Add support to append to argLine from CLI
1760b3d
@CodenameGHOST007
Update JAVA_HOME condition
1740e07
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

Run Gluten Clickhouse CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surnaik surnaik surnaik approved these changes

@zhztheplayer zhztheplayer Awaiting requested review from zhztheplayer

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
BUILD CORE works for Gluten Core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CodenameGHOST007 @surnaik @zhztheplayer