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

Simplify shim classloader logic [databricks] #3756

Merged
merged 3 commits into from
Oct 6, 2021
Merged

Simplify shim classloader logic [databricks] #3756

merged 3 commits into from
Oct 6, 2021

Conversation

gerashegalov
Copy link
Collaborator

@gerashegalov gerashegalov commented Oct 6, 2021 •
edited by tgravescs
Loading

Signed-off-by: Gera Shegalov gera@apache.org

@gerashegalov
Simplify shim classloader logic
ce248db 
- uprev spark320 breaking DiskManager changes
- use the Serializer instance to find mutable classloader
- make the the update logic oblivious to the executor/driver side

Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov gerashegalov self-assigned this Oct 6, 2021
@gerashegalov gerashegalov added bug Something isn't working build Related to CI / CD or cleanly building Spark 3.2+ labels Oct 6, 2021
@gerashegalov gerashegalov added this to the Oct 4 - Oct 15 milestone Oct 6, 2021
@gerashegalov
Restore ExecutorClassLoader treatment for userClassPathFirst
bd84e8b 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov gerashegalov changed the title Simplify shim classloader logic Simplify shim classloader logic [databricks] Oct 6, 2021
@gerashegalov
Copy link
Collaborator Author

build

@tgravescs
Copy link
Collaborator

07:52:27 [ERROR] [Error] /home/jenkins/agent/workspace/jenkins-rapids_premerge-github-2928/sql-plugin/src/main/320/scala/org/apache/spark/rapids/shims/v2/storage/ShimDiskBlockManager.scala:29: too many arguments (3) for constructor DiskBlockManager: (conf: org.apache.spark.SparkConf, deleteFilesOnStop: Boolean)org.apache.spark.storage.DiskBlockManager

tgravescs
tgravescs reviewed Oct 6, 2021
View reviewed changes
Copy link
Collaborator

@tgravescs tgravescs left a comment

Choose a reason for hiding this comment

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

can you please update the description with what the issues were that this is fixing. I'm guessing the build env doesn't have the newer spark jars yet to pick up that change.

dist/pom.xml Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/ShimLoader.scala Outdated Show resolved Hide resolved
@tgravescs
Copy link
Collaborator

note the 3.2 deploy is running now so should be under an hour

abellina
abellina reviewed Oct 6, 2021
View reviewed changes
Copy link
Collaborator

@abellina abellina left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, just had a minor nit that should be skipped if no other changes are made to this PR. The description of the PR is not clear on what this is solving (are we just simplifying or are we fixing an issue.. other than the DiskBlockManager breaking change).

@tgravescs
Copy link
Collaborator

build

@gerashegalov
Review comments
52de827 
Signed-off-by: Gera Shegalov <gera@apache.org>
@abellina
Copy link
Collaborator

abellina commented Oct 6, 2021

build

tgravescs
tgravescs approved these changes Oct 6, 2021
View reviewed changes
Copy link
Collaborator

@tgravescs tgravescs left a comment

Choose a reason for hiding this comment

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

Description still needs updating.

@tgravescs tgravescs merged commit a19486f into NVIDIA:branch-21.10 Oct 6, 2021
@tgravescs tgravescs mentioned this pull request Oct 6, 2021
Closed
@gerashegalov gerashegalov deleted the improveClassloaderHack branch October 6, 2021 22:46
@gerashegalov gerashegalov mentioned this pull request Oct 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgravescs tgravescs tgravescs approved these changes

@abellina abellina abellina approved these changes

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

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

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

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

Assignees

@gerashegalov gerashegalov

Labels
bug Something isn't working build Related to CI / CD or cleanly building Spark 3.2+
Projects
None yet
Milestone
Oct 4 - Oct 15
Development

Successfully merging this pull request may close these issues.

[BUG] Databricks class cast exception failure
3 participants
@gerashegalov @tgravescs @abellina