Skip to content

Conversation

@devaraj-kavali
Copy link

@devaraj-kavali devaraj-kavali commented Jan 26, 2019

What changes were proposed in this pull request?

As part of the SPARK-22404 PR, base log url formation has been changed for unmanaged am which causes to include key is also part of the base url. This PR corrects the addition of key into base url.

How was this patch tested?

Verified the url manually and see that this has been corrected with the PR change.

Before this PR the base log url is like,

  • Unmanaged AM enabled:
    http://ip:8042/node/containerlogs/container_1544212645385_0252_01_000002/(USER, devaraj)

  • Unmanaged AM disabled:
    http://ip:8042/node/containerlogs/container_1544212645385_0254_01_000002/(SPARK_USER, devaraj)

With the PR change, base log url is like this:

  • Unmanaged AM enabled/disabled:
    http://ip:8042/node/containerlogs/container_1544212645385_0253_01_000002/devaraj

@SparkQA
Copy link

SparkQA commented Jan 26, 2019

Test build #101707 has finished for PR 23659 at commit a5bcfa1.

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

// Add log urls
container.foreach { c =>
sys.env.filterKeys(_.endsWith("USER")).foreach { user =>
sys.env.get("SPARK_USER").orElse(sys.env.get("USER")).foreach { user =>
Copy link
Contributor

@HeartSaVioR HeartSaVioR Jan 26, 2019

Choose a reason for hiding this comment

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

So our preference would be SPARK_USER -> USER, right? I also have to apply this to #23260 given that I'm refactoring the this code block in #23260.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe explicitly set the SPARK_USER env variable to the value of Utils.getCurrentUserName? Instead of trying to figure out what's the correct value from the env. (USER could be wrong if you have kerberos, for example.)

@HeartSaVioR
Copy link
Contributor

Please add [FOLLOWUP] after [YARN] to PR's title.

@devaraj-kavali devaraj-kavali changed the title [SPARK-26737][YARN] Executor/Task STDERR & STDOUT log urls are not correct in Yarn deployment mode [SPARK-26737][YARN][FOLLOWUP] Executor/Task STDERR & STDOUT log urls are not correct in Yarn deployment mode Jan 26, 2019
@HeartSaVioR
Copy link
Contributor

Ah sorry I thought you didn't file a new issue and submit a PR to SPARK-22404. Could you please remove `[FOLLOWUP]? Sorry to bother you.

@HyukjinKwon HyukjinKwon changed the title [SPARK-26737][YARN][FOLLOWUP] Executor/Task STDERR & STDOUT log urls are not correct in Yarn deployment mode [SPARK-26737][YARN] Executor/Task STDERR & STDOUT log urls are not correct in Yarn deployment mode Jan 27, 2019
@vanzin
Copy link
Contributor

vanzin commented Jan 28, 2019

#23260 will actually fix this too, so might as well just stick to that one patch. I'll close this one and put a note on the bug when that patch is merged.

@vanzin vanzin closed this Jan 28, 2019
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