Skip to content
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.

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

Fix the pid occupied check when use bookkeeper-daemon.sh start or stop #3113

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

Nicklee007
Copy link
Contributor

@Nicklee007 Nicklee007 commented Mar 16, 2022

Master Issue: #3112

Motivation

Fix the failed pid occupied check. we'll fail when use bookkeeper-daemon.sh start or stop bookie, after the last time we exit the bookie direct kill or the bookie occurred non-normal exit, then the bin/bookkeeper-bookie.pid are retained, and the pid in the file is occupied by the thread in other progress.

Changes

Change the pid occupied check from 'kill -0 $pid' to 'ps -p $pid'. The both will return true when the pid is occupied by one progress, but 'kill -0 $pid' will return true and the 'ps -p $pid' will return false when the pid is occupied by one progress's sub thread.

StackOverflow discussion:
https://stackoverflow.com/questions/30958964/why-do-kill-0-pid-echo-and-ps-ppid-echo-sometimes-differ

@eolivelli
Copy link
Contributor

on which platforms did you test this ?
Did you test this on Linux and MacOS ?

@eolivelli
Copy link
Contributor

Ci is not running for some reason
@nicoloboschi do you have any ideas ?

@nicoloboschi
Copy link
Contributor

perhaps: https://www.githubstatus.com/incidents/fpk08rxnqjz2

can you try to close and reopen the PR ?

@eolivelli eolivelli closed this Mar 17, 2022
@eolivelli eolivelli reopened this Mar 17, 2022
@Nicklee007
Copy link
Contributor Author

on which platforms did you test this ? Did you test this on Linux and MacOS ?

@eolivelli I test the case in on Linux and MacOS. On Linux the 'kill -0 $pid' and 'ps -p $pid' performance diff as the discussion (https://stackoverflow.com/questions/30958964/why-do-kill-0-pid-echo-and-ps-ppid-echo-sometimes-differ);On MacOs, the both exhibit the same result;

StevenLuMT

This comment was marked as duplicate.

@hangc0276 hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 29, 2022
@StevenLuMT
Copy link
Member

fix old workflow,please see #3455 for detail

@shoothzj
Copy link
Member

@Nicklee007 Could you please rebase the code? I will push on this forward. Thanks :)

Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

it look very nice
but I have a question:
how to verify the correctness of this shell change? there is no relevant testcase for the shell, right?

@shoothzj
Copy link
Member

@StevenLuMT Currently we don't have shell test suite, and it's difficult to built. So I think manual testing is enough

@StevenLuMT
Copy link
Member

@StevenLuMT Currently we don't have shell test suite, and it's difficult to built. So I think manual testing is enough

ok,LGTM

@shoothzj shoothzj merged commit 3ed93a0 into apache:master Apr 29, 2024
20 checks passed
@hangc0276 hangc0276 modified the milestones: 4.17.0, 4.18.0 May 25, 2024
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
apache#3113)

Master Issue: apache#3112

### Motivation

Fix the failed pid occupied check. we'll fail when use bookkeeper-daemon.sh start or stop bookie, after the last time we exit the bookie direct kill or the bookie occurred non-normal exit, then the bin/bookkeeper-bookie.pid are retained, and the pid in the file is occupied by the thread in other progress.

### Changes
Change the pid occupied check from 'kill -0 $pid' to 'ps -p $pid'. The both will return true when the pid is occupied by one progress, but 'kill -0 $pid' will return true and the 'ps -p $pid' will return false when the pid is occupied by one progress's sub thread.

StackOverflow discussion:
https://stackoverflow.com/questions/30958964/why-do-kill-0-pid-echo-and-ps-ppid-echo-sometimes-differ

Co-authored-by: nicklixinyang <nicklixinyang@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants