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

Update the scala repl loader to avoid issues with broadcast. #3498

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Sep 15, 2021

This fixes #3482

Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
@revans2 revans2 added the bug Something isn't working label Sep 15, 2021
@revans2 revans2 added this to the Sep 13 - Sep 24 milestone Sep 15, 2021
@revans2 revans2 self-assigned this Sep 15, 2021
@revans2
Copy link
Collaborator Author

revans2 commented Sep 15, 2021

build

// Next we should check for the scala built in REPL interpreter class loader.
// This happens on the driver side as a part of the REPL. It does not use a
// MutableURLClassLoader, but has essentially done the same thing.
Option(contextClassLoader).foreach { loader =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, but I would generalize the pattern matching above and make it another case to avoid have another place to add the same url and pluginClassLoader assingnment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried it, but it felt less clear and didn't save much. Did you mean you wanted to call addURL by reflection all of the time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

along these lines, unconditional by reflection was not my initial idea but I think it's reasonable here and will make the code even simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried and I ran into a few issues with using reflection for both paths. I don't really feel like spending more time to debug something that works just to save a few lines of code. So if you are okay with it I think I will just keep it as is.

@revans2 revans2 merged commit 371a81b into NVIDIA:branch-21.10 Sep 15, 2021
@revans2 revans2 deleted the repl_loader_fix branch September 15, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ClassNotFound error when running a job
2 participants