-
Notifications
You must be signed in to change notification settings - Fork 198
[ARUON #1541] Add work flow for iceberg #1541 #1536
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
base: master
Are you sure you want to change the base?
Conversation
6d6c737 to
3f5d8df
Compare
3f5d8df to
78cd486
Compare
| javaver: [ "11"] | ||
| scalaver: [ "2.12" ] | ||
| module: [ "thirdparty/auron-iceberg" ] | ||
| sparkver: [ "spark-3.4", "spark-3.5" ] |
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 slightly inconsistent with the rest of the inputs into the matrix. WDYT about this
| sparkver: [ "spark-3.4", "spark-3.5" ] | |
| sparkver: [ "3.4", "3.5" ] |
and then we can update L61 to
run: ./build/mvn -B test -X -pl ${{ matrix.module }} -am -Pscala-${{ matrix.scalaver }} -Piceberg-${{ matrix.iceberg }} -Pspark-${{ matrix.sparkver }} -Prelease
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 slightly inconsistent with the rest of the inputs into the matrix. WDYT about this
and then we can update L61 to
run: ./build/mvn -B test -X -pl ${{ matrix.module }} -am -Pscala-${{ matrix.scalaver }} -Piceberg-${{ matrix.iceberg }} -Pspark-${{ matrix.sparkver }} -Prelease
@ShreyeshArangath Thanks for reviewing the code. I followed the current workflow used in other projects to keep it consistent with them.
.github/workflows/iceberg.yml
Outdated
| fail-fast: false | ||
| matrix: | ||
| iceberg: [ "1.9" ] | ||
| javaver: [ "11"] |
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.
Any reason for not having JDK 17 here?
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.
Any reason for not having JDK 17 here?
@ShreyeshArangath DONE
|
@guixiaowen, I think the TPC-DS test failure is transient. Do you mind pushing up an empty commit so that we can merge this in? |
Which issue does this PR close?
Closes #1541
Rationale for this change
What changes are included in this PR?
Associate with issue #1472
Add work flow for iceberg
Are there any user-facing changes?
How was this patch tested?