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.

Sign up for GitHub

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

Jump to bottom

Replaced spark3xx-common references to spark-shared [databricks] #11066

Merged
merged 9 commits into from
Jun 28, 2024
Merged

Replaced spark3xx-common references to spark-shared [databricks] #11066

merged 9 commits into from
Jun 28, 2024

Conversation

razajafri
Copy link
Collaborator

@razajafri razajafri commented Jun 14, 2024 •
edited
Loading

In #10993 we made changes to add support for Spark 4.0.0. This PR removes some left-over usage of spark3xx-common.

@razajafri
Signing off
cd17dd0 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri razajafri mentioned this pull request Jun 14, 2024
Merged
gerashegalov
gerashegalov reviewed Jun 15, 2024
View reviewed changes
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

Update this comment as well

spark-rapids/dist/scripts/binary-dedupe.sh

Lines 131 to 133 in 2f3c0c2

# spark30x-common
# spark31x-common
# spark32x-common

@razajafri
Copy link
Collaborator Author

Update this comment as well

spark-rapids/dist/scripts/binary-dedupe.sh

Lines 131 to 133 in 2f3c0c2

# spark30x-common
# spark31x-common
# spark32x-common

Since this is a TODO and you wrote this comment do you think naming them spark32x-shared etc in future makes sense? Or were you thinking of renaming them to something different?

@gerashegalov
Copy link
Collaborator

gerashegalov commented Jun 15, 2024 •
edited
Loading

Since this is a TODO and you wrote this comment do you think naming them spark32x-shared etc in future makes sense? Or were you thinking of renaming them to something different?

spark32x-shared is a good way. However, on a second thought: to follow best practice let us delete the TODO and I'll file an issue with the idea

@tgravescs
Copy link
Collaborator

can we please have a description on this as to what the problem is/what this relates to and what this is fixing?

Why are we changing name from common to shared, seems arbitrary?

jlowe
jlowe reviewed Jun 17, 2024
View reviewed changes
build/coverage-report Outdated Show resolved Hide resolved
jenkins/spark-premerge-build.sh Outdated Show resolved Hide resolved
@mythrocks mythrocks mentioned this pull request Jun 24, 2024
Merged
@razajafri
Copy link
Collaborator Author

can we please have a description on this as to what the problem is/what this relates to and what this is fixing?

Why are we changing name from common to shared, seems arbitrary?

I have updated the description, does that give enough context or should I add something more?

@razajafri
Copy link
Collaborator Author

@jlowe @gerashegalov @tgravescs please take another look. This needs to be merged very soon

@razajafri
Copy link
Collaborator Author

build

jlowe
jlowe previously approved these changes Jun 24, 2024
View reviewed changes
@razajafri
Copy link
Collaborator Author

premerge is failing but I don't see anything on the page besides While reading class directory: /var/jenkins/jobs/rapids_premerge-github/builds/9620/jacoco/classes

re-kicking

@razajafri
Copy link
Collaborator Author

build

@razajafri
Copy link
Collaborator Author

Jacoco coverage report is still failing. I'm still trying to root cause this.

@razajafri
Copy link
Collaborator Author

build

@razajafri razajafri changed the title Replaced spark3xx-common references to spark-shared Replaced spark3xx-common references to spark-shared [databricks] Jun 27, 2024
@razajafri
Copy link
Collaborator Author

build

@razajafri
Copy link
Collaborator Author

Updated the title to make sure all Databricks versions pass.

jlowe
jlowe reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Need scala2.13 poms regenerated

@razajafri
Copy link
Collaborator Author

build

@razajafri razajafri merged commit 3b6c5cd into NVIDIA:branch-24.08 Jun 28, 2024
45 checks passed
@razajafri razajafri deleted the SP-9259-binar-dedupe branch June 28, 2024 06:15
@sameerz sameerz added the Spark 4.0+ Spark 4.0+ issues label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerashegalov gerashegalov gerashegalov left review comments

@jlowe jlowe jlowe approved these changes

@revans2 revans2 Awaiting requested review from revans2 revans2 is a code owner

@tgravescs tgravescs Awaiting requested review from tgravescs tgravescs is a code owner

@GaryShen2008 GaryShen2008 Awaiting requested review from GaryShen2008 GaryShen2008 is a code owner

@NvTimLiu NvTimLiu Awaiting requested review from NvTimLiu NvTimLiu is a code owner

Assignees
No one assigned
Labels
Spark 4.0+ Spark 4.0+ issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@razajafri @gerashegalov @tgravescs @jlowe @sameerz