[SPARK-4913] Fix incorrect event log path

[viirya]

SPARK-2261 uses a single file to log events for an app. eventLogDir in ApplicationDescription is replaced with eventLogFile. However, ApplicationDescription in SparkDeploySchedulerBackend is initialized with SparkContext's eventLogDir. It is just the log directory, not the actual log file path. Master.rebuildSparkUI can not correctly rebuild a new SparkUI for the app.

Because the ApplicationDescription is remotely registered with Master and the app's id is then generated in Master, we can not get the app id in advance before registration. So the received description needs to be modified with correct eventLogFile value.

[ash211]

ok to test

[SparkQA]

Test build #24687 has finished for PR 3755 at commit b5730a1.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[JoshRosen]

/cc @vanzin @andrewor14 (#1222)

[viirya]

Looks like the test failed not because of this PR. Please test it again.

[vanzin]

Hmm, guess I missed this in my testing.

Anyway, I think this is the wrong place for the fix. The right fix in my view should be in SparkDeploySchedulerBackend, where the application description is created.

[vanzin]

Here's what I think is a better approach, feel free to use / adapt it:
https://gist.github.com/vanzin/e1910b11ce00630fe9d4

[andrewor14]

retest this please

@vanzin I believe we separated the definition of isEventLogEnabled from that of eventLogger because of the following initialization ordering constraint. We need to pass the base log dir to the standalone master when we start the scheduler, but at that time we don't have an app ID yet so we can't specify the full path.

[vanzin]

I see. Hmm. That sucks. :-/ A comment there would help at least, but even better would be to avoid this tight coupling altogether.

[andrewor14]

Hey @viirya thanks for fixing this. I believe this PR works, but right fix here is to change the eventLogFile field back to an eventLogDir (because it refers to the base logging directory, not the application-specific one), and in Master#rebuildSparkUI append the application ID to the logging directory as we have done before #1222. Additionally we might want to detect the case when the application is still in progress and show a different message to warn the user that the application didn't exit cleanly.

[vanzin]

Andrew's suggestion sounds good. Long term, I think it would be better to send this log path later (as some sort of "application stopping" message maybe?), instead of trying to send it up front like this.

[andrewor14]

For instance... https://github.com/andrewor14/spark/compare/fix-event-log-suggestion

[vanzin]

I think using EventLoggingListener.getLogPath() is safer (as in the current diff) due to the escaping it does. Just in case.

[andrewor14]

Ah yes I forgot that exists. I have updated the branch.

[viirya]

Thanks @andrewor14 @vanzin. I made corresponding revision.

[SparkQA]

Test build #24732 has finished for PR 3755 at commit 5e0ea35.

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

[vanzin]

LGTM. Thanks!

[JoshRosen]

This looks good to me, too, so I'm going to merge this into master (1.3.0) and use the commit message to close #3777 since it's a duplicate. Thanks!

[andrewor14]

Looks great. Thanks for adding the warning about inprogress

[viirya]

Thanks for your suggestion too.