Skip to content

[SPARK-24203][core] Make executor's bindAddress configurable - #26331

Closed
nishchalv wants to merge 1 commit into
apache:masterfrom
nishchalv:SPARK-29670
Closed

[SPARK-24203][core] Make executor's bindAddress configurable#26331
nishchalv wants to merge 1 commit into
apache:masterfrom
nishchalv:SPARK-29670

Conversation

@nishchalv

@nishchalvnishchalv commented Oct 30, 2019

Copy link
Copy Markdown

What changes were proposed in this pull request?

With this change, executor's bindAddress is passed as an input parameter for RPCEnv.create.
A previous PR #21261 which addressed the same, was using a Spark Conf property to get the bindAddress which wouldn't have worked for multiple executors.
This PR is to enable anyone overriding CoarseGrainedExecutorBackend with their custom one to be able to invoke CoarseGrainedExecutorBackend.main() along with the option to configure bindAddress.

Why are the changes needed?

This is required when Kernel-based Virtual Machine (KVM)'s are used inside Linux container where the hostname is not the same as container hostname.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Tested by running jobs with executors on KVMs inside a linux container.

@dongjoon-hyun

dongjoon-hyun commented Oct 30, 2019

Copy link
Copy Markdown
Member

Hi, @nishchalv .
Could you rebase to the master? GitHub Action complains that the PR fails to build.

@nishchalv

nishchalv commented Oct 30, 2019

Copy link
Copy Markdown
Author

@dongjoon-hyun fixed the build.

@nishchalv
nishchalvforce-pushed the SPARK-29670 branch 5 times, most recently from 0937237 to 62974f8CompareOctober 31, 2019 00:16
@dbtsai

dbtsai commented Nov 1, 2019

Copy link
Copy Markdown
Member

Jenkins, test this please.

@dbtsaidbtsai changed the title [SPARK-29670][core] Make executor's bindAddress configurable[SPARK-24203][Core] Make executor's bindAddress configurableNov 1, 2019
@nishchalvnishchalv changed the title [SPARK-24203][Core] Make executor's bindAddress configurable[SPARK-24203][core] Make executor's bindAddress configurableNov 1, 2019
@SparkQA

SparkQA commented Nov 1, 2019

Copy link
Copy Markdown

Test build #113070 has finished for PR 26331 at commit 62974f8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@holdenk

holdenk commented Nov 1, 2019

Copy link
Copy Markdown
Contributor

Oh interesting, this could potentially help with an issue I've been seeing in Kubeflow with Spark under certain scenarios. I'll try and take a proper look next week if no one else has a chance.

@holdenk

holdenk commented Nov 1, 2019

Copy link
Copy Markdown
Contributor

Let's CC the folks from the previous PR for their thoughts too: @lukmajercak & @bjliu & @vanzin

@holdenk

holdenk commented Nov 1, 2019

Copy link
Copy Markdown
Contributor

@nishchalv can you run the Kubernetes integration tests? I know this isn't directly changing anything there so I think think this is going to trigger the K8s CI but I think with a core change like this it's good to make sure the different cluster managers work as much as possible

dongjoon-hyun
dongjoon-hyun reviewed Nov 1, 2019
View reviewed changes
Comment threadcore/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

Arguments(driverUrl, executorId, hostname, cores, appId, workerUrl,
if (bindAddress == null) {
bindAddress = hostname

@dongjoon-hyundongjoon-hyunNov 1, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need a new test case for this code path, @nishchalv .

@nishchalvnishchalvNov 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@dongjoon-hyun Added couple of test cases.

@dongjoon-hyundongjoon-hyunNov 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

dongjoon-hyun
dongjoon-hyun reviewed Nov 1, 2019
View reviewed changes
Comment threadcore/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala
-1,
executorConf,
new SecurityManager(executorConf),
numUsableCores = 0,

@dongjoon-hyundongjoon-hyunNov 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this line in this PR?

@dbtsaidbtsaiNov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is needed unless we add extra method in RpcEnv object.

@lukmajercak

lukmajercak commented Nov 5, 2019

Copy link
Copy Markdown

Thanks guys for picking this up. We internally patched this a while ago and have been running with this change since.

vanzin
vanzin reviewed Nov 5, 2019
View reviewed changes

@vanzinvanzin left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks ok, if you ignore how weird it is to have an internal API that isn't used by anyone, except people external to the project extending internal classes...

Also, Dongjoon's question.

Comment threadcore/src/main/scala/org/apache/spark/SparkEnv.scala Outdated
ioEncryptionKey: Option[Array[Byte]],
isLocal: Boolean): SparkEnv = {
createExecutorEnv(conf, executorId, hostname,
hostname, numCores, ioEncryptionKey, isLocal);

@vanzinvanzinNov 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

drop the semi-colon

@nishchalvnishchalvNov 8, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, will do.

@dbtsai

dbtsai commented Nov 9, 2019•
edited
Loading

Copy link
Copy Markdown
Member

LGTM except the extra test requested by @dongjoon-hyun Thanks!

@dbtsai
dbtsai self-requested a review November 13, 2019 19:58
dbtsai
dbtsai approved these changes Nov 13, 2019
View reviewed changes

@dbtsaidbtsai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Waiting the build.

dongjoon-hyun
dongjoon-hyun approved these changes Nov 13, 2019
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. Thank you for updating, @nishchalv .

@dongjoon-hyun

dongjoon-hyun commented Nov 13, 2019

Copy link
Copy Markdown
Member

Retest this please.

jiangxb1987
jiangxb1987 approved these changes Nov 13, 2019
View reviewed changes

@jiangxb1987jiangxb1987 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@dbtsai

dbtsai commented Nov 13, 2019

Copy link
Copy Markdown
Member

Merged into master. Thanks!

@SparkQA

SparkQA commented Nov 13, 2019

Copy link
Copy Markdown

Test build #113726 has finished for PR 26331 at commit c2a2b95.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gedeh

gedeh commented May 19, 2021

Copy link
Copy Markdown

Hi, any plan to merge this to Spark 2.4? Thanks!

@gedehgedeh mentioned this pull request May 22, 2021
Closed
@laocoder2014laocoder2014 mentioned this pull request Mar 25, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vanzinvanzinvanzin left review comments
@dbtsaidbtsaidbtsai approved these changes
@jiangxb1987jiangxb1987jiangxb1987 approved these changes
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes

Assignees

No one assigned

Labels

SPARK CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@nishchalv@dongjoon-hyun@dbtsai@SparkQA@holdenk@lukmajercak@gedeh@vanzin@jiangxb1987