Skip to content

Conversation

@JoshRosen
Copy link
Contributor

Currently, the driver's executorId is set to <driver>. This choice of ID was present in older Spark versions, but it has started to cause problems now that executorIds are used in more contexts, such as Ganglia metric names or driver thread-dump links the web UI. The angle brackets must be escaped when embedding this ID in XML or as part of URLs and this has led to multiple problems:

The simplest solution seems to be to change this id to something that does not contain any special characters, such as driver.

I'm not sure whether we can perform this change in a patch release, since this ID may be considered a stable API by metrics users, but it's probably okay to do this in a major release as long as we document it in the release notes.

@JoshRosen
Copy link
Contributor Author

/cc @marmbrus @jerryshao

@SparkQA
Copy link

SparkQA commented Apr 5, 2015

Test build #29732 has started for PR 5372 at commit 7ff12e0.

@SparkQA
Copy link

SparkQA commented Apr 5, 2015

Test build #29732 has finished for PR 5372 at commit 7ff12e0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29732/
Test PASSed.

@pwendell
Copy link
Contributor

pwendell commented Apr 5, 2015

Do we need to modify isDriver to accept the old version of the identifier as well? For instance, if a newer version of the History server is playing logs from an older version of Spark, where the string "" is coded in the event data for the addition of an executor.

@JoshRosen
Copy link
Contributor Author

Good call; I'll update BlockManagerId.isDriver and add a test. Note that we don't need to update SparkContext.getExecutorThreadDump since thread-dumps are disabled in the HistoryServer.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29734 has started for PR 5372 at commit 0c5d04b.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29734 has finished for PR 5372 at commit 0c5d04b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29734/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be interpreted as implying it is only needed for tests, however, this is actually legitimately needed for reading old event logs. It might make more sense to just say it's needed for backwards compatibility.

@pwendell
Copy link
Contributor

pwendell commented Apr 6, 2015

Left a very minor comment but otherwise LGTM

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29742 has started for PR 5372 at commit 42d3c10.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29742 has finished for PR 5372 at commit 42d3c10.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29742/
Test PASSed.

@JoshRosen
Copy link
Contributor Author

Good call on the original comment wording being potentially confusing. I've updated this as suggested.

I'm going to merge this into master (1.4.0). Thanks!

@asfgit asfgit closed this in a0846c4 Apr 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants