AvanishShukla20 commented on PR #3253: URL: https://github.com/apache/hugegraph/pull/3253#issuecomment-6334524894
> The earlier review comments below still apply at [860ec0f](https://github.com/apache/hugegraph/commit/860ec0faef003fa2737a27c5fbbc0d6666bc0145); this head does not fix them. > > * Important: The comment says this stop keeps the monitor, but stop-hugegraph.sh only reads options with `getopts "i:m:"`, and getopts stops at the positional `false`. With `-i /x/pid false` it sets the PID file and leaves `CLOSE_MONITOR=true`, so this call runs stop-monitor.sh and removes the cron job that is doing the restart. After one health-check restart the server is no longer monitored. The bug exists at the base too, but this PR edits this line and its comment. Please pass `-m false` instead of the bare `false`. ([#3253 (comment)](https://github.com/apache/hugegraph/pull/3253#discussion_r4217946999)) > * Important: The removal key now always has single quotes around it, but the previous start-monitor.sh wrote the unquoted form `... && $TOP/bin/monitor-hugegraph.sh`. After an in-place upgrade with an existing monitor job, `grep -F` does not match, `crontab_remove` returns 0, stop-hugegraph.sh prints that the monitor was closed, and the old cron line can restart the server within a minute. The new start-monitor.sh also does not detect the old line and adds a second job. Please match both the quoted and unquoted forms when removing (and when checking for duplicates in crontab_append), and add an old-format-to-new-format case to test-cron-quote.sh. ([#3253 (comment)](https://github.com/apache/hugegraph/pull/3253#discussion_r4217946991)) @bitflicker64 The changes intended for monitor scripts, start-monitor.sh will be covered as next steps after fixture failures and other feedbacks are resolved. The head [860ec0f](https://github.com/apache/hugegraph/commit/860ec0faef003fa2737a27c5fbbc0d6666bc0145) does not cover it fully as described in [implementation summary](https://github.com/apache/hugegraph/pull/3253#issuecomment-6301938776). Thank you so much for pointing out the fixture failures.Review feedbacks from imbajin have also been noted . They will be addressed in the upcoming commit. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
