PDGGK commented on code in PR #18546:
URL: https://github.com/apache/iotdb/pull/18546#discussion_r3893844310
##########
scripts/sbin/stop-edge.sh:
##########
@@ -31,9 +41,14 @@ is_same_edge_home() {
return 0
;;
*)
- return 1
;;
esac
+ # Fall back to comparing resolved paths, so that a start-edge.sh invoked
with
+ # IOTDB_HOME pointing at a symlink is still recognised here.
+ local home="${command_line#*-DIOTDB_HOME=}"
+ home="${home%% -D*}"
+ [ -n "$home" ] && [ "$home" != "$command_line" ] || return 1
+ [ "$(resolve_home "$home")" = "${IOTDB_HOME_RESOLVED}" ]
Review Comment:
You are right, and thank you for running it. I reproduced it before changing
anything: a synthetic process with `cwd` in `edge-a` and `-DIOTDB_HOME=.` on
its
command line, stopped from `edge-b` -- the base script correctly declines
it, my
fallback resolved the `.` against the stopping shell and terminated it.
I have taken the first of your two suggestions. `start-edge.sh` now
normalises
`IOTDB_HOME` to a physical absolute path before exporting it, so the value
handed
to the JVM identifies the installation on its own:
```
started through the symlink, before -DIOTDB_HOME=$ROOT/iotdb
started through the symlink, now
-DIOTDB_HOME=$ROOT/apache-iotdb-2.0.11-SNAPSHOT-edge-bin
```
I kept an absolute-path guard in the `stop-edge.sh` fallback as well, rather
than
relying only on the launcher. A process started by an earlier `start-edge.sh`
during an upgrade can still carry a relative value, and for those the
fallback now
declines instead of resolving against our own directory -- the same outcome
as the
base script. That leaves resolving against the process's working directory
unnecessary, which I was reluctant to do since it needs `lsof` or `/proc`
and a
wrong answer there puts us back to stopping the wrong process.
Two things about the normalisation that I would rather state than leave to be
found. It changes the path string that appears in `ps`, in the console
message and
in the logs -- on this machine `/tmp` becomes `/private/tmp` -- which is the
point
of the change, but anything grepping for the old string is affected. And if
the
`cd -P` cannot run, `IOTDB_HOME` keeps the value it had, which for the unset
case
is still relative; that instance then falls to the literal comparison in
`stop-edge.sh`, which is today's behaviour. A fallback to a logical `cd`
does not
help here: both still have to reach the directory, so it fails on the same
inputs.
Both of your cases are in the regression set now, with the version you
reviewed as
a control so they are shown to fail without the change.
##########
scripts/sbin/stop-edge.sh:
##########
@@ -20,7 +20,17 @@
# Stop IoTDB Edge (the merged ConfigNode + DataNode process).
-IOTDB_HOME="$(cd "$(dirname "$0")"/.. && pwd)"
+if [ -z "${IOTDB_HOME}" ]; then
+ IOTDB_HOME="$(cd "$(dirname "$0")"/.. && pwd)"
+fi
+
+# Resolve symlinks and relative segments so that the same installation reached
+# through a different path still compares equal.
+resolve_home() {
+ (cd "$1" 2>/dev/null && pwd -P) || printf '%s' "$1"
Review Comment:
Confirmed, and thank you -- this one reproduces as pure path arithmetic,
without
needing a process. With `x/link` pointing at `y/sub` and a process home of
`x/link/../edge`, the physical target is `y/edge`, and the old helper
returned
`x/edge`:
```
process home $ROOT/x/link/../edge
physically $ROOT/y/edge
old resolve_home $ROOT/x/edge <- wrong installation
cd -P version $ROOT/y/edge
```
Fixed in 739dae2 with `cd -P -- "$1"`, and both of your cases are in the
regression set now:
```
scenario base previous now
-------------------------------------------------- ------ -------- ------
[P1] -DIOTDB_HOME=. in A, stopped from B keep stopped keep
[P2] home x/link/../edge, stopped from x/edge keep stopped keep
[P2] home x/link/../edge, stopped from y/edge - - stopped
symlink home, stopped from the real path keep stopped stopped
```
The "previous" column is the version you reviewed, so the two cases do fail
without the change rather than passing for an unrelated reason. The four
start/stop environment combinations from the PR description still behave the
same, checked against real Edge processes with 10710 confirmed listening
before
each stop.
One limit of the fallback that I should state rather than leave for you to
find:
it locates the end of the value by the next ` -D`, which `start-edge.sh`
always
emits after `-DIOTDB_HOME`. A hand-built command line that ends with
`-DIOTDB_HOME=<symlink>` is not matched by the fallback. The failure
direction is
the safe one -- the extraction keeps the trailing arguments, the resolution
fails,
and the process is simply not matched -- so that case behaves as it does
today. I have noted it in a comment above the
fallback.
On the PID file being removed before `stop_edge_process` succeeds: I agree,
and I
will open that separately as you suggested, since it changes the exit-code
contract and is worth reviewing on its own.
--
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]
