-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-12386] [core] Fix NPE when spark.executor.port is set. #10339
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
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.
could you use port directly here since rpcEnv doesn't use it in this case?
|
LGTM pending tests |
|
Test build #47862 has finished for PR 10339 at commit
|
|
I don't know... it's longer than the one-liner I was told to expect. :) |
|
retest this please |
I deleted just one line! |
|
retest this please |
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.
@markhamstra @vanzin we can make this a real oneliner if we use Option :)
val actorSystemPort = if (port == 0) 0 else Option(rpcEnv.address).map(_.port + 1).getOrElse(port)
not sure if that's easier or harder to read. Either way is fine.
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.
It's simpler: val actorSystemPort = if (port == 0 || rpcEnv.address == null) port else rpcEnv.address.port + 1
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.
+1 to @zsxwing
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.
Still doesn't fit in one line, though. But it's simpler.
|
LGTM |
|
Test build #47867 has finished for PR 10339 at commit
|
|
flakiness? retest this please |
|
Test build #47874 has finished for PR 10339 at commit
|
|
Test build #47872 has finished for PR 10339 at commit
|
|
Merging into master 1.6 thanks! |
Author: Marcelo Vanzin <[email protected]> Closes #10339 from vanzin/SPARK-12386. (cherry picked from commit d1508dd) Signed-off-by: Andrew Or <[email protected]>
|
Test build #47881 has finished for PR 10339 at commit
|
|
Test build #47885 has finished for PR 10339 at commit
|
No description provided.