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

Add Spark 3.2.2, 3.3.0, and 3.3.1 #614

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

cphbrt
Copy link
Contributor

@cphbrt cphbrt commented Nov 16, 2022

This is my first contribution to this repository, so please advise on any style or process mistakes.

An existing PR is open for 3.3.0 but is stalled: #605

The ./gradlew clean run completes successfully on my machine with this PR's changes, but I had to use the command docker run -d -p 27017:27017 --name=mongo arm64v8/mongo:4.0.28 to setup Mongo on my machine, which differs from the example on the README because I am on Apple silicon and Docker Hub doesn't have a mongo 3.2 for arm64.

I did not test if the sdk install spark 3.3.1 will succeed with this PR's changes, because I am not sure how to hook up my local SDKMAN with the mongoDB container...

So, tl;dr, I followed the patterns I see in Git closely. Happy to change, test, or add anything you advise.

Thanks!

PS: I got the new Spark version URLs from here: https://archive.apache.org/dist/spark/

You may notice that the end of the URL switched from 3.2.tgz to 3.tgz at Spark 3.3.X. It looks comparably harmless to the change from 2.7.tgz to 3.2.tgz that occurred several commits ago in this repo.

@GurRonenExplorium
Copy link

Would love this as well! 👍

Copy link
Contributor

@hgeraldino hgeraldino left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @cphbrt

The methods on these migration classes should be treated as immutable; that is, instead of editing the existing migrations (order 17, 19 and so on) you should instead add new ones, following the same sequences.

Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

LGTM

@eddumelendez
Copy link
Member

@hgeraldino I think it is fine. The approach we have been following is removing the old one and add new ones just like the PR is (at least for java candidates), in order to avoid having a big file with ton of migrations. Or maybe this only applied to java 🤔 ? /cc @marc0der

@cphbrt
Copy link
Contributor Author

cphbrt commented Nov 21, 2022

Looking specifically at git log src/main/scala/io/sdkman/changelogs/SparkMigrations.scala, it appears that commit 801f6d4 by @marc0der was the biggest step in removing old migrations. That pattern was continued in 4d293e0 by @chethanuk-plutoflume. However, in the currently open PR from @chethanuk-plutoflume (#605), @chethanuk-plutoflume left the previous migration present and suggested we remove it in a subsequent PR.

That appears to be the history. I don't know enough about the setup to determine if it was a good idea to remove old migrations.

I'll wait to make changes until y'all talk through a plan. I could restore all the migrations removed from src/main/scala/io/sdkman/changelogs/SparkMigrations.scala in 801f6d4 and 4d293e0 if desired.

@eddumelendez eddumelendez merged commit 22ab8f4 into sdkman:master Nov 21, 2022
@cphbrt cphbrt mentioned this pull request Nov 3, 2023
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.

4 participants