-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-41571: [Java] Revert GH-41307 (#41309) #41628
Conversation
|
@github-actions crossbow submit test--spark- java |
Revision: 8285a2b Submitted crossbow builds: ursacomputing/crossbow @ actions-a804e6c132 |
@danepitkin @vibhatha if the java-jars and spark jobs are this finicky, they really should be part of the regular CI and not Crossbow |
It also appears the C++ build broke again |
@lidavidm +1 to including java-jars with regular CIs. It has been failing for many cases very recently. |
@felipecrv have you come across this issue: https://github.com/ursacomputing/crossbow/actions/runs/9054922130/job/24875318571#step:8:2503
|
@vibhatha I never seen it because I never build Gandiva. |
No review? I'm going to close this PR soon if it looks like we don't want it @vibhatha @danepitkin |
@lidavidm I will review this today. Shall we rebase since there were some changes in Java and CIs. |
@@ -274,6 +288,7 @@ | |||
<plugin> | |||
<groupId>org.cyclonedx</groupId> | |||
<artifactId>cyclonedx-maven-plugin</artifactId> | |||
<version>2.7.11</version> |
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.
@lidavidm just curious, I am comparing this PR agains: #41309.
And We the following dependency in the main
<plugin>
<groupId>org.cyclonedx</groupId>
<artifactId>cyclonedx-maven-plugin</artifactId>
<version>2.8.0</version>
</plugin>
And we had the following in the said PR.
<plugin>
<groupId>org.cyclonedx</groupId>
<artifactId>cyclonedx-maven-plugin</artifactId>
<version>2.7.11</version>
</plugin>
And in here we have 2.7.11
, any specific reason we picked 2.7.11? not 2.8.0
?
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.
I'm not particularly bothered so long as CI passes. We can let dependabot upgrade it again.
@@ -22,7 +22,9 @@ | |||
<description>JMH Performance benchmarks for other Arrow libraries.</description> | |||
|
|||
<properties> | |||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> |
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.
Here in the previous PR the following component has been removed, but we haven't reverted it back here.
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration combine.self="override">
<compilerVersion>${javac.target}</compilerVersion>
<source>${javac.target}</source>
<target>${javac.target}</target>
</configuration>
</plugin>
And in the main we have
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<annotationProcessorPaths combine.children="append">
<path>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<version>${jmh.version}</version>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
So I hope this is okay?
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.
I'm not particularly bothered so long as CI passes.
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.
@lidavidm I added a few comments. Shall we rebase the PR and run the CIs once more?
@github-actions crossbow submit -g java |
Revision: 8285a2b Submitted crossbow builds: ursacomputing/crossbow @ actions-4bdc9eb706 |
@lidavidm the java-jars CI failures are from a previously resolved issued. |
This reverts commit 9090e67.
@github-actions crossbow submit -g java |
Revision: 3c561a2 Submitted crossbow builds: ursacomputing/crossbow @ actions-057b79da1a |
Looks like Spark is fixed but Dataset is now broken. |
@github-actions crossbow submit -g java |
Revision: dd15529 Submitted crossbow builds: ursacomputing/crossbow @ actions-a5a11e35cd |
Yes, a test is failing. Let me check if it is failing in other PRs. |
@github-actions crossbow submit java-jars |
Revision: 290cdba Submitted crossbow builds: ursacomputing/crossbow @ actions-d1d168f413
|
@github-actions crossbow submit -g java |
Revision: 290cdba Submitted crossbow builds: ursacomputing/crossbow @ actions-d90628c846 |
@lidavidm seems like java-jars is passing 🎉 |
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.
CI's are passing. And PR LGTM!
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit e3cd0ae. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…1628) ### Rationale for this change The commit in question caused a lot of CI issues ### Are these changes tested? N/A ### Are there any user-facing changes? N/A * GitHub Issue: apache#41571 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
The commit in question caused a lot of CI issues
Are these changes tested?
N/A
Are there any user-facing changes?
N/A