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.

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

chore: Make it possible to run 'make benchmark-%' using jvm 17+ #823

Conversation

eejbyfeldt
Copy link
Contributor

@eejbyfeldt eejbyfeldt commented Aug 13, 2024

Which issue does this PR close?

N/A.

Rationale for this change

When using jvm 17+ spark needs extra jvm args to avoid getting errors like

[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.2.0:java (default-cli) on project comet-spark-spark3.4_2.12: An exception occurred while executing the Java class. class org.apache.spark.storage.StorageUtils$ (in unnamed module @0x35b75242) cannot access class sun.nio.ch.DirectBuffer (in module java.base) because module java.base does not export sun.nio.ch to unnamed module @0x35b75242 -> [Help 1]

What changes are included in this PR?

These args are already present inside the main pom.xml. To avoid duplicating the args we using maven to extract them. In order avoid slowing down the Makefile the args are defined as a function and therefore only evaluated when they are needed.

How are these changes tested?

Manually running the make commands using jvm 17.

When using jvm 17+ spark needs extra jvm args to avoid getting errors
like
```
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.2.0:java (default-cli) on project comet-spark-spark3.4_2.12: An exception occurred while executing the Java class. class org.apache.spark.storage.StorageUtils$ (in unnamed module @0x35b75242) cannot access class sun.nio.ch.DirectBuffer (in module java.base) because module java.base does not export sun.nio.ch to unnamed module @0x35b75242 -> [Help 1]
```

These args are already present inside the main pom.xml. To avoid
duplicating the args we using maven to extract them. In order avoid
slowing down the Makefile the args are defined as a function and
therefore only evaluated when they are needed.
@eejbyfeldt eejbyfeldt changed the title Make it possible to run 'make benchmark-%' using jvm 17+ chore: Make it possible to run 'make benchmark-%' using jvm 17+ Aug 13, 2024
@eejbyfeldt eejbyfeldt marked this pull request as ready for review August 13, 2024 13:08
@@ -80,7 +84,7 @@ release:
release-nogit:
cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES) -Dmaven.gitcommitid.skip=true
benchmark-%: clean release
cd spark && COMET_CONF_DIR=$(shell pwd)/conf MAVEN_OPTS='-Xmx20g' ../mvnw exec:java -Dexec.mainClass="$*" -Dexec.classpathScope="test" -Dexec.cleanupDaemonThreads="false" -Dexec.args="$(filter-out $@,$(MAKECMDGOALS))" $(PROFILES)
benchmark-%: release
Copy link
Member

Choose a reason for hiding this comment

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

+1 for removing clean

@andygrove andygrove merged commit a0734dc into apache:main Aug 13, 2024
75 of 76 checks passed
himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024
When using jvm 17+ spark needs extra jvm args to avoid getting errors
like
```
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.2.0:java (default-cli) on project comet-spark-spark3.4_2.12: An exception occurred while executing the Java class. class org.apache.spark.storage.StorageUtils$ (in unnamed module @0x35b75242) cannot access class sun.nio.ch.DirectBuffer (in module java.base) because module java.base does not export sun.nio.ch to unnamed module @0x35b75242 -> [Help 1]
```

These args are already present inside the main pom.xml. To avoid
duplicating the args we using maven to extract them. In order avoid
slowing down the Makefile the args are defined as a function and
therefore only evaluated when they are needed.

(cherry picked from commit a0734dc)
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.

3 participants