magibney commented on code in PR #1328:
URL: https://github.com/apache/solr/pull/1328#discussion_r1095906127
##########
solr/bin/solr:
##########
@@ -2158,6 +2158,35 @@ function mk_writable_dir() {
fi
}
+# Check whether at least one of lsof/netstat/ss are available
+function get_port_tool() {
+ for tool in 'lsof' 'netstat' 'ss'; do
+ which $tool &> nul
Review Comment:
Looks like it might be preferable to use `command` in place of `which`? --
(there's
[precedent](https://github.com/apache/solr/blob/e35347e7cc12d91af91cb38f9cef43ccc7a7b40e/solr/bin/solr#L125-L126)
in this script already)
##########
solr/bin/solr:
##########
@@ -2312,13 +2341,13 @@ function start_solr() {
echo ""
fi
# no lsof on cygwin though
- if lsof -v 2>&1 | grep -q revision; then
+ if [ -n "$(get_port_tool)" ]; then
Review Comment:
rather than seeking the port tool a second time (calling `get_port_tool()`
from within the `check_port_in_use()` function), would it be just as clean/easy
to assign the output of `get_port_tool` to a variable, check that variable for
`[ -n "$PORT_TOOL" ]`, and pass `$PORT_TOOL` as an arg to `check_port_in_use()`?
Fine as-is, but seems there'd be very little downside to this change?
##########
solr/bin/solr:
##########
@@ -2312,13 +2341,13 @@ function start_solr() {
echo ""
fi
# no lsof on cygwin though
- if lsof -v 2>&1 | grep -q revision; then
+ if [ -n "$(get_port_tool)" ]; then
echo -n "Waiting up to $SOLR_START_WAIT seconds to see Solr running on
port $SOLR_PORT"
# Launch in a subshell to show the spinner
(loops=0
while true
do
- running=$(lsof -t -PniTCP:$SOLR_PORT -sTCP:LISTEN || :)
+ running=$(check_port_in_use $SOLR_PORT)
Review Comment:
The ` || : ` in the initial command is curious. I infer that this protects
against the possibility that a failed `lsof` command might still write to
stdout, which would get captured into the `running` variable. Whatever the
reason, might we want to either add ` || : ` to the `ss` and `netstat` variants
of the port command, or move the ` || : ` back to the outer level here
(ensuring that failed invocations of `lsof`/`ss`/`netstat` propagate failure
exit codes via `check_port_in_use()`?)
##########
solr/bin/solr:
##########
@@ -2158,6 +2158,35 @@ function mk_writable_dir() {
fi
}
+# Check whether at least one of lsof/netstat/ss are available
+function get_port_tool() {
+ for tool in 'lsof' 'netstat' 'ss'; do
+ which $tool &> nul
+ if [[ $? -eq 0 ]]; then
+ echo $tool
+ return
+ fi
+ done
+}
+
+# Will get the PID of the java process running on the specified port, if one
is running
+# Otherwise empty result. Use get_port_tool to check whether lsof/ss/netstat
is installed first
+function check_port_in_use() {
+ local port=$1
+
+ case $(get_port_tool) in
+ lsof)
+ lsof -iTCP:${port} 2>/dev/null | grep "LISTEN" | grep -Eow
"java\s*?[0-9]+" | grep -Eow [0-9]+
+ ;;
+ ss)
+ ss -ntpa 2>/dev/null | grep ":${port} " | grep 'LISTEN ' | grep -Eow
"\"java\",(pid=)?[0-9]+?" | grep -Eow "[0-9]+"
+ ;;
+ netstat)
+ netstat -nlp 2>/dev/null | grep ":${port} " | grep -Eow " [0-9]+/java" |
grep -Eow "[0-9]+"
+ ;;
Review Comment:
Analogous to the change above (restoring the more targeted use of `lsof`,
avoiding the need for `grep`), I wonder if there are more targeted (but still
portable) versions of this command for `ss` and `netstat`?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
