[SPARK-4595][Core] Fix MetricsServlet not work issue

[jerryshao]

MetricsServlet handler should be added to the web UI after initialized by MetricsSystem, otherwise servlet handler cannot be attached.

[SparkQA]

Test build #23822 has started for PR 3444 at commit f779fe0.

• This patch merges cleanly.

[SparkQA]

Test build #23822 has finished for PR 3444 at commit f779fe0.

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

[AmplabJenkins]

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

[JoshRosen]

I'm finally getting around to reviewing this now. Is there a good way to test this? What's the error message / symptom thatI should look for in the buggy version of this code?

[JoshRosen]

Ah, this was actually a pretty nasty bug since it seems to silently fail!

I'm fine merging this patch to fix this issue, but I wonder whether there are other changes that we could make to MetricsSystem in order to prevent these sorts of issues. For example, it looks like MetricsSystem has a bunch of public methods like getServletHandlers that don't indicate when they're safe to call (e.g. that they are only safe to call after the MetricsSystem has been started). Similarly, there are a bunch of public methods like registerSources, buildRegistryName, and registerSinks() that should be private.

I'd be happy to do this cleanup here, but maybe we should just merge this as-is then do the cleanup in a subsequent PR.

Actually, let's try this: I'll submit a PR to your PR which does the cleanup in MetricsSystem.scala in order to prevent these errors from occurring silently.

[JoshRosen]

I've opened a PR against this PR in order to add defensive checks to MetricsSystem, which would have prevented the issue fixed by this PR: jerryshao#10

@jerryshao would be great if you could take a look at that and merge it into this PR branch if it looks fine to you (I've opened it against this branch).

[jerryshao]

Thanks a lot Josh, I will merge into this PR and update the code :)

[SparkQA]

Test build #24520 has started for PR 3444 at commit 434d17e.

• This patch merges cleanly.

[SparkQA]

Test build #24520 has finished for PR 3444 at commit 434d17e.

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

[AmplabJenkins]

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

[JoshRosen]

I tested this out and it looks good to me, so I'm going to merge this into master, branch-1.2, and branch-1.1. Thanks for fixing this!

[JoshRosen]

Actually, it looks like this issue doesn't affect 1.1.1, since I'm able to browse to /metrics/json after starting spark-shell. Therefore, I'm not merging this into branch-1.1 (it would have tons of merge-conflicts there, too).