[SPARK-4832][Deploy]some other processes might take the daemon pid

[WangTaoTheTonic]

Some other processes might use the pid saved in pid file. In that case we should ignore it and launch daemons.

JIRA is down for maintenance. I will file one once it return.

[SparkQA]

Test build #24397 has started for PR 3683 at commit f36cfb4.

• This patch merges cleanly.

[SparkQA]

Test build #24397 has finished for PR 3683 at commit f36cfb4.

• 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/24397/
Test PASSed.

[srowen]

I agree this looks like a better test, but isn't it redundant with kill?
Or: in case you have detected that the PID file existed but isn't the expected process, log a message?
You could use ps -p for consistency in both cases.

[WangTaoTheTonic]

Yep it is redundant with kill. And I will handle the case that we might kill other processes by mistake.

[SparkQA]

Test build #24405 has started for PR 3683 at commit cf4ecc6.

• This patch merges cleanly.

[SparkQA]

Test build #24406 has started for PR 3683 at commit 8befee7.

• This patch merges cleanly.

[SparkQA]

Test build #24405 has finished for PR 3683 at commit cf4ecc6.

• 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/24405/
Test PASSed.

[SparkQA]

Test build #24406 has finished for PR 3683 at commit 8befee7.

• 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/24406/
Test PASSed.

[WangTaoTheTonic]

@srowen About the process isn't the one we expected, is it necessary to log a message? Ignoring it would be better cause it doesn't do any hurt I think.

[srowen]

It means the process didn't exit cleanly before, and the PID file is invalid. It can't hurt to note that. The PID file should probably be deleted too.

[WangTaoTheTonic]

If the process with the pid stored in pid file is not spark daemon, it cannot prove spark daemon didn't exist cleanly but just some other process take that pid after spark daemon ends.
I don't think the pid file should be deleted as we cannot make sure the process exit cleanly.
I'm not sure I understand what you mean totally. Correct me if I am wrong.

[srowen]

Hm, shouldn't the process or its script clean up a PID file? If the daemon is not running at that process ID, when would you want to leave a PID file saying it is?

[WangTaoTheTonic]

In case that user might start a daemon that is already running, we use pid file to check and give a message. Another case is that daemon might not exit after user stop it, according to the pid file we can prompt them.

[srowen]

I think we can finish and merge this. The current logic of the script is "if anything is running at the PID given in the file, fail". Your change is "if the Spark daemon we expect is running at the PID given in the file, fail" -- meaning in the case where somehow the PID files wasn't cleaned up and another process has that PID, we continue.

That's good. I had suggested cleaning up the PID file when it's known it's not the Spark daemon. But I see that in the start case, the PID file is overwritten anyway. In the stop case, it's not cleared, but, maybe that's not so important.

So, I think that is actually already addressed. Could I get @nchammas to render an opinion on the bash syntax -- are we not doing back ticks? Otherwise I think it's a good one to merge.

[nchammas]

Bash style: Use $(...) instead of backticks, which are discouraged.

Also, quote the output of the subshell, as well as the variable argument to cat:

TARGET_ID="$(cat "$pid")"

[nchammas]

Generally looks good. Left a bunch of feedback about Bash style and protecting against word splitting bugs.

[SparkQA]

Test build #27422 has started for PR 3683 at commit daa86a1.

• This patch merges cleanly.

[WangTaoTheTonic]

@nchammas Updated as the feedback. Could you check it again, please? Thanks!

[nchammas]

LGTM Bash-wise.

[SparkQA]

Test build #27422 has finished for PR 3683 at commit daa86a1.

• 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/27422/
Test PASSed.