Skip to content

[Spark-5889] Remove pid file after stopping service. #4676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

[Spark-5889] Remove pid file after stopping service. #4676

wants to merge 5 commits into from

Conversation

@zhzhan
Copy link
Contributor

@zhzhan zhzhan commented Feb 18, 2015

Currently the pid file is not deleted, and potentially may cause some problem after service is stopped. The fix remove the pid file after service stopped.

@zhzhan zhzhan changed the title [Spark 5889] Remove pid file after stopping service. [Spark-5889] Remove pid file after stopping service. Feb 18, 2015
@SparkQA
Copy link

SparkQA commented Feb 18, 2015

Test build #27685 has started for PR 4676 at commit bfabd91.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 18, 2015

Test build #27685 has finished for PR 4676 at commit bfabd91.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Feb 18, 2015

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

srowen
srowen reviewed Feb 19, 2015
View reviewed changes
sbin/spark-daemon.sh Outdated
Copy link
Member

@srowen srowen Feb 19, 2015

Choose a reason for hiding this comment

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

Concretely, can there be an if statement here to check whether kill succeeded, and only rm if so?

Copy link
Contributor

@andrewor14 andrewor14 Feb 19, 2015

Choose a reason for hiding this comment

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

a more concise way to do this is kill ... && rm -rf $pid I think

Copy link
Member

@srowen srowen Feb 19, 2015

Choose a reason for hiding this comment

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

Yeah, it occurs to me that this doesn't even guarantee the process was killed, but that the signal was sent. Well, I kind of like kill ... && rm -f $pid and would vote for merging that.

@zhzhan
Copy link
Contributor Author

zhzhan commented Feb 19, 2015

@srowen @andrewor14 Changed to kill "$TARGET_ID" && rm -f $pid. Thanks.

@SparkQA
Copy link

SparkQA commented Feb 19, 2015

Test build #27747 has started for PR 4676 at commit b4c009e.

  • This patch merges cleanly.

@srowen
Copy link
Member

srowen commented Feb 19, 2015

@zhzhan I apologize for the extra round trip on this simple change, but it occurs to me that we should quote "$pid". We should probably do it elsewhere in this an other scripts as @nchammas reminds us, but I imagine it's especially important as the argument to rm. Just in case there's... I don't know, a /opt/very/important/Project file and for some reason the PID path is /opt/very/important/Project Data/pid!

@AmplabJenkins
Copy link

AmplabJenkins commented Feb 19, 2015

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

@srowen
Copy link
Member

srowen commented Feb 19, 2015

(Test failure looks like a Jenkins network problem; ignore it)

@zhzhan
Copy link
Contributor Author

zhzhan commented Feb 19, 2015

@srowen No problem. Thanks for the explanation, and that's a valid one.

@SparkQA
Copy link

SparkQA commented Feb 19, 2015

Test build #27753 has started for PR 4676 at commit eb01be1.

  • This patch merges cleanly.

asfgit pushed a commit that referenced this pull request Feb 19, 2015
@zhzhan @srowen
[Spark-5889] Remove pid file after stopping service.
ff8976e 
Currently the pid file is not deleted, and potentially may cause some problem after service is stopped. The fix remove the pid file after service stopped.

Author: Zhan Zhang <[email protected]>

Closes #4676 from zhzhan/spark-5889 and squashes the following commits:

eb01be1 [Zhan Zhang] solve review comments
b4c009e [Zhan Zhang] solve review comments
018110a [Zhan Zhang] spark-5889: remove pid file after stopping service
088d2a2 [Zhan Zhang] squash all commits
c1f1fa5 [Zhan Zhang] test

(cherry picked from commit ad6b169)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in ad6b169 Feb 19, 2015
asfgit pushed a commit that referenced this pull request Feb 19, 2015
@zhzhan @srowen
[Spark-5889] Remove pid file after stopping service.
18fbed5 
Currently the pid file is not deleted, and potentially may cause some problem after service is stopped. The fix remove the pid file after service stopped.

Author: Zhan Zhang <[email protected]>

Closes #4676 from zhzhan/spark-5889 and squashes the following commits:

eb01be1 [Zhan Zhang] solve review comments
b4c009e [Zhan Zhang] solve review comments
018110a [Zhan Zhang] spark-5889: remove pid file after stopping service
088d2a2 [Zhan Zhang] squash all commits
c1f1fa5 [Zhan Zhang] test

(cherry picked from commit ad6b169)
Signed-off-by: Sean Owen <[email protected]>
@SparkQA
Copy link

SparkQA commented Feb 20, 2015

Test build #27753 has finished for PR 4676 at commit eb01be1.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Feb 20, 2015

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

@SparkQA
Copy link

SparkQA commented Feb 20, 2015

Test build #27747 timed out for PR 4676 at commit b4c009e after a configured wait of 120m.

@AmplabJenkins
Copy link

AmplabJenkins commented Feb 20, 2015

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

markhamstra pushed a commit to markhamstra/spark that referenced this pull request Feb 24, 2015
@zhzhan @markhamstra
[Spark-5889] Remove pid file after stopping service.
84f2865 
Currently the pid file is not deleted, and potentially may cause some problem after service is stopped. The fix remove the pid file after service stopped.

Author: Zhan Zhang <[email protected]>

Closes apache#4676 from zhzhan/spark-5889 and squashes the following commits:

eb01be1 [Zhan Zhang] solve review comments
b4c009e [Zhan Zhang] solve review comments
018110a [Zhan Zhang] spark-5889: remove pid file after stopping service
088d2a2 [Zhan Zhang] squash all commits
c1f1fa5 [Zhan Zhang] test

(cherry picked from commit ad6b169)
Signed-off-by: Sean Owen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zhzhan @SparkQA @AmplabJenkins @srowen @andrewor14