[SPARK-16148] [Scheduler] Allow for underscores in TaskLocation in the Executor ID #13858

tmagrino · 2016-06-22T21:21:23Z

What changes were proposed in this pull request?

Previously, the TaskLocation implementation would not allow for executor ids which include underscores. This tweaks the string split used to get the hostname and executor id, allowing for underscores in the executor id.

This addresses the JIRA found here: https://issues.apache.org/jira/browse/SPARK-16148

This is moved over from a previous PR against branch-1.6: #13857

How was this patch tested?

Ran existing unit tests for core and streaming. Manually ran a simple streaming job with an executor whose id contained underscores and confirmed that the job ran successfully.

This is my original work and I license the work to the project under the project's open source license.

srowen · 2016-06-23T06:07:41Z

Jenkins test this please

SparkQA · 2016-06-23T08:04:40Z

Test build #61098 has finished for PR 13858 at commit b497dc9.

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

tmagrino · 2016-06-28T00:37:08Z

CCing some people who were involved in previous commits for this part of the code, @tdas @zsxwing. Does this look good?

zsxwing · 2016-06-28T00:58:08Z

@tmagrino could you add a simple test here:

spark/core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

Line 804 in 2821589

    
           assert(TaskLocation("executor_host1_3") === ExecutorCacheTaskLocation("host1", "3"))

?

srowen · 2016-06-28T06:28:37Z

I had an outstanding comment from the previous PR too: #13857 (comment)

tmagrino · 2016-06-28T08:10:59Z

My apologies @srowen, I missed the comment somehow! I refactored a little bit to make it more obvious what's going on there (using stripPrefix as you suggested).

Also updated the comment to indicate the string format which includes the executor id.

@srowen

In response to @srowen and @zsxwing's comments.

srowen · 2016-06-28T08:22:10Z

You can always revert the commits in question and push, which become new commits, but that's fine. They're all squashed in the end. You can also manually squash and force-push if you like.

tmagrino · 2016-06-28T08:22:57Z

I opted to force push a cleaned up version, thanks!

srowen · 2016-06-28T08:26:29Z

core/src/main/scala/org/apache/spark/scheduler/TaskLocation.scala

-        if (splits.length != 3) {
+        val hostAndExecutorId = str.stripPrefix(executorLocationTag)
+        val splits = hostAndExecutorId.split("_", 2)
+        if (splits.length != 2) {


Not important, but you could just use require here.

srowen · 2016-06-28T09:12:22Z

core/src/main/scala/org/apache/spark/scheduler/TaskLocation.scala

-        new ExecutorCacheTaskLocation(splits(1), splits(2))
+        val hostAndExecutorId = str.stripPrefix(executorLocationTag)
+        val splits = hostAndExecutorId.split("_", 2)
+        require(splits.length == 2, "Illegal executor location format: " + str)


You can use string interpolation here but not worth changing if that's the only thing left

srowen · 2016-06-28T09:12:30Z

Jenkins test this please

SparkQA · 2016-06-28T11:08:38Z

Test build #61371 has finished for PR 13858 at commit ee4708d.

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

zsxwing · 2016-06-28T20:35:36Z

LGTM. Merging to master, 2.0 and 1.6. Thanks!

… Executor ID ## What changes were proposed in this pull request? Previously, the TaskLocation implementation would not allow for executor ids which include underscores. This tweaks the string split used to get the hostname and executor id, allowing for underscores in the executor id. This addresses the JIRA found here: https://issues.apache.org/jira/browse/SPARK-16148 This is moved over from a previous PR against branch-1.6: #13857 ## How was this patch tested? Ran existing unit tests for core and streaming. Manually ran a simple streaming job with an executor whose id contained underscores and confirmed that the job ran successfully. This is my original work and I license the work to the project under the project's open source license. Author: Tom Magrino <[email protected]> Closes #13858 from tmagrino/fixtasklocation. (cherry picked from commit ae14f36) Signed-off-by: Shixiong Zhu <[email protected]>

… Executor ID ## What changes were proposed in this pull request? Previously, the TaskLocation implementation would not allow for executor ids which include underscores. This tweaks the string split used to get the hostname and executor id, allowing for underscores in the executor id. This addresses the JIRA found here: https://issues.apache.org/jira/browse/SPARK-16148 This is moved over from a previous PR against branch-1.6: apache#13857 ## How was this patch tested? Ran existing unit tests for core and streaming. Manually ran a simple streaming job with an executor whose id contained underscores and confirmed that the job ran successfully. This is my original work and I license the work to the project under the project's open source license. Author: Tom Magrino <[email protected]> Closes apache#13858 from tmagrino/fixtasklocation. (cherry picked from commit ae14f36) Signed-off-by: Shixiong Zhu <[email protected]> (cherry picked from commit 0cb06c9)

rxin · 2016-06-29T19:18:18Z

core/src/main/scala/org/apache/spark/scheduler/TaskLocation.scala

+        val hostAndExecutorId = str.stripPrefix(executorLocationTag)
+        val splits = hostAndExecutorId.split("_", 2)
+        require(splits.length == 2, "Illegal executor location format: " + str)
+        val Array(host, executorId) = splits


it's usually better to just do splits(0) and splits(1) rather than using pattern matching here.

tmagrino added 3 commits June 28, 2016 01:19

Fixed split command in TaskLocation to allow for _ in executor id.

c0b108b

Also updated the comment to indicate the string format which includes the executor id.

Code style fix.

c2f0a2c

Refactored code and added test case.

0aed80a

In response to @srowen and @zsxwing's comments.

tmagrino force-pushed the fixtasklocation branch from ec4f920 to 0aed80a Compare June 28, 2016 08:20

srowen reviewed Jun 28, 2016
View reviewed changes

Tweak to use require and array deconstruction for cleaner code.

ee4708d

srowen reviewed Jun 28, 2016
View reviewed changes

asfgit closed this in ae14f36 Jun 28, 2016

rxin reviewed Jun 29, 2016
View reviewed changes

[SPARK-16148] [Scheduler] Allow for underscores in TaskLocation in the Executor ID #13858

[SPARK-16148] [Scheduler] Allow for underscores in TaskLocation in the Executor ID #13858

Uh oh!

Conversation

tmagrino commented Jun 22, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented Jun 23, 2016

Uh oh!

SparkQA commented Jun 23, 2016

Uh oh!

tmagrino commented Jun 28, 2016

Uh oh!

zsxwing commented Jun 28, 2016

Uh oh!

srowen commented Jun 28, 2016

Uh oh!

tmagrino commented Jun 28, 2016

Uh oh!

srowen commented Jun 28, 2016

Uh oh!

tmagrino commented Jun 28, 2016

Uh oh!

srowen Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented Jun 28, 2016

Uh oh!

SparkQA commented Jun 28, 2016

Uh oh!

zsxwing commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin Jun 29, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zsxwing commented Jun 28, 2016 •

edited

Loading