-
Notifications
You must be signed in to change notification settings - Fork 27
Initial commit for Iceberg catalog migrator #1
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
Conversation
|
Should there be a Failure: |
e1fdcb1 to
904ea26
Compare
|
@mansehajsingh: Thanks for taking a look. I have addressed the comments. |
snazy
left a comment
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.
Overall the PR LGTM.
My request for changes is due to the addition of a .jar file and mention of the donation.
I also think, that the migrator should actually live in its own directory, as this repository is rather generic ("tools"). I propose to either move everything to iceberg-catalog-migrator/ or to have a dedicated Git repo for it.
gradle/wrapper/gradle-wrapper.jar
Outdated
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.
The gradle-wrapper.jar should be removed. Binary files, especially those having code, are not good practice.
There's a mechanism in the main repo to work around that.
.github/workflows/main.yml
Outdated
| uses: gradle/actions/setup-gradle@v4 | ||
|
|
||
| - name: Build & Check | ||
| run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal --scan |
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.
| run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal --scan | |
| run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal |
No scan yet - see apache/polaris#475
.github/workflows/main.yml
Outdated
| # since the `nessieQuarkusApp` gradle plugin expects the below variable | ||
| env: | ||
| JDK17_HOME: ${{ env.JAVA_HOME_17_X64 }} |
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 should be superfluous
NOTICE
Outdated
| The Apache Software Foundation (http://www.apache.org/). | ||
|
|
||
| The initial code for the Polaris project was donated | ||
| to the ASF by Snowflake Inc. (https://www.snowflake.com/) copyright 2024. |
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 needs to be adopted
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.
good catch. Updated.
| @@ -0,0 +1,94 @@ | |||
| @rem | |||
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.
The gradlew.bat file needs to be removed to work with the non-gradle-wrapper-jar mechanism
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.
Please remove this file
904ea26 to
a149ccf
Compare
|
@snazy: thanks for the review. I have updated the gradle stuff to be similar to polaris main repo and verified |
|
And regarding this
The modules related to catalog migrator is under |
jbonofre
left a comment
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.
First quick pass, overall ok, a couple of nits.
I'm also checking build and generated artifacts (in terms of legal).
|
By the way, I would propose to have it in a dedicated repo (as we will have more tools coming in the repo, e.g. benchmark). |
a149ccf to
1ef40ee
Compare
We agreed on |
1ef40ee to
8ec369a
Compare
+1 to this - would be super helpful if other additions to the repo were able to share these and deduplicate the work needed to bootstrap a new tool. |
|
Hi everyone, Thanks for the reviews. I have addressed all the comments. I would like to squash the PR once I have approval to add |
|
Also merging this can help for #2 to avoid duplicate work of introducing build and infra related stuffs. |
mansehajsingh
left a comment
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.
Accidentally submitted approval on my other account.
8ec369a to
535d4b6
Compare
|
I squashed the commit to add co-author by in the commit message. |
|
@snazy: Could you please take another look? |
535d4b6 to
4292ef4
Compare
snazy
left a comment
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.
Given that the polaris-tools repository is meant to host multiple and independent projects, I think that all those sub-projects should live in their own subfolder and be completely independent.
Therefore, everything, except the .github/ folder and the README.md + SECURITY.md files should go under the iceberg-catalog-migrator/ folder.
| @@ -0,0 +1,94 @@ | |||
| @rem | |||
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.
Please remove this file
- Copied Main branch of nessie catalog migrator #bca310f - Replace headers - Rename packages - Update security.md - Remove automated release workflow related to projectnessie accounts - Clean up Dremio keyword - Adopt to polaris tools repo This PR also contains the contributions from Co-authored-by: Robert Stupp [email protected] Co-authored-by: Christopher Lambert [email protected] Co-authored-by: Harsh Maheshwari [email protected]
920518c to
09e2a1f
Compare
@snazy: Thanks again for the review. I was under the impression that each tool can share the same gradle and build stuffs. But if we are going with this approach is also fine for me as it can help in having individual release versions. I have updated the PR now. Thanks. |
80ed633 to
e7e3123
Compare
snazy
left a comment
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.
Hm, the build's failing:
FAILURE: Build failed with an exception.
* Where:
Settings file '/home/snazy/devel/polaris/polaris-tools/iceberg-catalog-migrator/settings.gradle.kts' line: 20
* What went wrong:
/home/snazy/devel/polaris/polaris-tools/iceberg-catalog-migrator/version.txt (No such file or directory)
That and a last name change and we should be good!
Don't forget to open an issue about add support for releases.
|
|
||
| val baseVersion = file("version.txt").readText().trim() | ||
|
|
||
| rootProject.name = "polaris-tools" |
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.
| rootProject.name = "polaris-tools" | |
| rootProject.name = "polaris-iceberg-catalog-migrator" |
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.
done
|
Issue #5 created |
e7e3123 to
d8b5101
Compare
I verified locally before pushing that builds are passing. But due to I think PR is ready. |
snazy
left a comment
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.
LGTM!
Please feel free to checkout this PR and run locally.
Verified
gradle clean buildand all the tests are passing.Changes:
mainbranch code form https://github.com/projectnessie/iceberg-catalog-migrator for donationorg.apache.polaris.iceberg.catalog.migratorTODO: (In the follow up PRs)
This PR also contains the contributions from
Co-authored-by: Robert Stupp [email protected]
Co-authored-by: Christopher Lambert [email protected]
Co-authored-by: Harsh Maheshwari [email protected]