Add shutdown.sh and match other shell script #7020

jgzl · 2021-05-26T12:18:02Z

remove shutdown.bat and update shutdown.sh,oapServiceShutdown.sh,webappServiceShutdown.sh match other shell script

…ppServiceShutdown.sh match other shell script

jgzl · 2021-05-26T12:34:08Z

codecov · 2021-05-26T12:46:00Z

Codecov Report

Merging #7020 (73a9ea8) into master (144210b) will decrease coverage by 8.45%.
The diff coverage is n/a.

❗ Current head 73a9ea8 differs from pull request most recent head c900957. Consider uploading reports for the commit c900957 to get more accurate results

@@             Coverage Diff              @@
##             master    #7020      +/-   ##
============================================
- Coverage     53.14%   44.69%   -8.46%     
+ Complexity     4217     2855    -1362     
============================================
  Files          1865     1852      -13     
  Lines         40319    39892     -427     
  Branches       4510     4658     +148     
============================================
- Hits          21429    17831    -3598     
- Misses        17836    21013    +3177     
+ Partials       1054     1048       -6

Impacted Files	Coverage Δ
...ywalking/apm/agent/core/plugin/EnhanceContext.java	`0.00% <0.00%> (-100.00%)`	⬇️
...walking/apm/agent/core/plugin/match/NameMatch.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ng/oap/server/core/analysis/config/NoneStream.java	`0.00% <0.00%> (-100.00%)`	⬇️
...rver/core/browser/source/BrowserErrorCategory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...core/browser/source/BrowserAppTrafficCategory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...gin/interceptor/enhance/MethodInterceptResult.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../parser/listener/DatabaseSlowStatementBuilder.java	`0.00% <0.00%> (-100.00%)`	⬇️
...torage/plugin/elasticsearch/base/StorageEsDAO.java	`0.00% <0.00%> (-100.00%)`	⬇️
...in/elasticsearch/base/TimeRangeIndexNameMaker.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../provider/parser/performance/PerfDataAnalyzer.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 288 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 144210b...c900957. Read the comment docs.

wu-sheng · 2021-05-26T13:05:46Z

@JaredTan95 @innerpeacez Any time to check this script locally? And give a review?

JaredTan95 · 2021-05-26T13:09:57Z

@JaredTan95 @innerpeacez Any time to check this script locally? And give a review?

Okay, I'll check it locally these days.

dist-material/bin/oapServiceShutdown.sh

kezhenxu94 · 2021-05-26T13:13:15Z

dist-material/bin/webappService.sh

 if [ $? -eq 0 ]; then
-    sleep 1
+  sleep 1
+  /bin/echo -n $! > "$UI_PID_FILE"


I suggest moving $! to line 39 and give it a variable, in case we add background commands between line 38 and this line, the pid will be wrong

I don't know how to give it a variable ,because $! is the PID of the last program your shell ran in the background

I don't know how to give it a variable ,because $! is the PID of the last program your shell ran in the background

I know. I mean pid=$!

dist-material/bin/webappServiceShutdown.sh

dmsolr · 2021-05-26T13:32:42Z

dist-material/bin/oapService.sh

 	echo "SkyWalking OAP started successfully!"
 else
 	echo "SkyWalking OAP started failure!"


Please keep 2 spaces as indentation.

Please keep 2 spaces as indentation.

I've completed

echo "SkyWalking OAP started successfully!" else echo "SkyWalking OAP started failure!"

echo "SkyWalking Web Application started successfully!" else echo "SkyWalking Web Application started failure!"

innerpeacez

It is suggested to stop the process by signal.

innerpeacez · 2021-05-26T13:42:10Z

dist-material/bin/oapServiceShutdown.sh

+OAP_PID_FILE="${SW_HOME}/bin/oap.pid"
+
+if [ -f $OAP_PID_FILE ]; then
+  kill -9 $(cat "$OAP_PID_FILE")


SIGNAL=${SIGNAL:-TERM}
PID=$(cat "$OAP_PID_FILE")
kill -s $SIGNAL $PID

I think this is a good suggestion

I've complete

SIGNAL=${SIGNAL:-TERM}
PID=$(cat "$OAP_PID_FILE")
kill -s $SIGNAL $PID

I've complete

OAP_PID_FILE="${SW_HOME}/bin/oap.pid" if [ -f "$OAP_PID_FILE" ]; then SIGNAL=${SIGNAL:-TERM} PID=$(cat "$OAP_PID_FILE") kill -s "$SIGNAL" "$PID" rm "$OAP_PID_FILE" echo 'SkyWalking OAP stopped successfully!' else echo 'SkyWalking OAP not exist(could not find file $OAP_PID_FILE)!' fi

What I mean is: Let the user control the SIGNAL used by himself in a variable way, we only provide a default SIGNAL.

kezhenxu94 · 2021-05-27T02:09:39Z

dist-material/bin/oapService.sh

@@ -40,10 +40,14 @@ OAP_OPTIONS=" -Doap.logDir=${OAP_LOG_DIR}"
 eval exec "\"$_RUNJAVA\" ${JAVA_OPTS} ${OAP_OPTIONS} -classpath $CLASSPATH org.apache.skywalking.oap.server.starter.OAPServerStartUp \
        2>${OAP_LOG_DIR}/oap.log 1> /dev/null &"

+PID="$!"
+OAP_PID_FILE="${OAP_HOME}/bin/oap.pid"
+
 if [ $? -eq 0 ]; then


$? should be also extracted into variable, now, and should be right after eval exec ...

like this

OAP_PID_FILE="${OAP_HOME}/bin/oap.pid" eval exec "\"$_RUNJAVA\" ${JAVA_OPTS} ${OAP_OPTIONS} -classpath $CLASSPATH org.apache.skywalking.oap.server.starter.OAPServerStartUp \ 2>${OAP_LOG_DIR}/oap.log 1> /dev/null & /bin/echo -n "$$" > "$OAP_PID_FILE""

like this

OAP_PID_FILE="${OAP_HOME}/bin/oap.pid" eval exec "\"$_RUNJAVA\" ${JAVA_OPTS} ${OAP_OPTIONS} -classpath $CLASSPATH org.apache.skywalking.oap.server.starter.OAPServerStartUp \ 2>${OAP_LOG_DIR}/oap.log 1> /dev/null & /bin/echo -n "$$" > "$OAP_PID_FILE""

It's ok, but you have to remove the PID file if $? != 0, right?

like this

OAP_PID_FILE="${OAP_HOME}/bin/oap.pid" eval exec "\"$_RUNJAVA\" ${JAVA_OPTS} ${OAP_OPTIONS} -classpath $CLASSPATH org.apache.skywalking.oap.server.starter.OAPServerStartUp \ 2>${OAP_LOG_DIR}/oap.log 1> /dev/null & /bin/echo -n "$$" > "$OAP_PID_FILE""

It's ok, but you have to remove the PID file if $? != 0, right?

I'm sorry , i didn't test before, this script can't get correct PID. tomcat shell script

else eval $_NOHUP "\"$_RUNJAVA\"" "\"$LOGGING_CONFIG\"" $LOGGING_MANAGER "$JAVA_OPTS" "$CATALINA_OPTS" \ -D$ENDORSED_PROP="\"$JAVA_ENDORSED_DIRS\"" \ -classpath "\"$CLASSPATH\"" \ -Dcatalina.base="\"$CATALINA_BASE\"" \ -Dcatalina.home="\"$CATALINA_HOME\"" \ -Djava.io.tmpdir="\"$CATALINA_TMPDIR\"" \ org.apache.catalina.startup.Bootstrap "$@" start \ >> "$CATALINA_OUT" 2>&1 "&" fi if [ ! -z "$CATALINA_PID" ]; then echo $! > "$CATALINA_PID" fi

@kezhenxu94 Because $! or $$ is used after exec, the PID obtained is the PID of the shell of the child process executed by exec, so the definition of $! can only be defined after the execution of exec. The reference tomcat's startup.sh is also written like this .

if [ ! -z "$CATALINA_PID" ]; then echo $! > "$CATALINA_PID" fi

wu-sheng · 2021-05-31T13:10:41Z

@jgzl Any update?

jgzl · 2021-06-01T06:48:12Z

@jgzl Any update?

I didn't update code

kezhenxu94 · 2021-06-01T07:06:52Z

@jgzl Any update?

I didn't update code

@jgzl We mean, are you still working on this PR or not? because there are some review comments not addressed yet, we can't merge it

wu-sheng · 2021-06-24T02:17:34Z

Closing due to no update.

jgzl added 5 commits May 25, 2021 21:21

add shutdown.sh

3d31fd9

add shutdown.sh(like tomcat startup.sh use pidfile)

9db8d63

add shutdown.sh(like tomcat startup.sh use pidfile)

d9a3465

remove shutdown.bat and update shutdown.sh,oapServiceShutdown.sh,weba…

7e42a98

…ppServiceShutdown.sh match other shell script

Merge remote-tracking branch 'origin/master'

49a06ba

wu-sheng requested review from innerpeacez and kezhenxu94 May 26, 2021 13:05

wu-sheng added backend OAP backend related. enhancement Enhancement on performance or codes labels May 26, 2021

wu-sheng requested a review from JaredTan95 May 26, 2021 13:05

kezhenxu94 reviewed May 26, 2021

View reviewed changes

dmsolr reviewed May 26, 2021

View reviewed changes

innerpeacez requested changes May 26, 2021

View reviewed changes

format shell script and use kill(default is kill -15)

f87f1dd

pull bot force-pushed the master branch from f87f1dd to 03294b8 Compare May 26, 2021 15:19

Merge remote-tracking branch 'origin/master'

f04ae1c

kezhenxu94 mentioned this pull request May 27, 2021

Add test for gracefully shutdown #7022

Closed

kezhenxu94 reviewed May 27, 2021

View reviewed changes

Merge branch 'master' into master

81bbf27

jgzl and others added 2 commits June 1, 2021 17:32

Merge branch 'apache:master' into master

d0b03fe

Merge branch 'master' into master

c900957

wu-sheng closed this Jun 24, 2021

wu-sheng added the invalid The description doesn't fit the case. label Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shutdown.sh and match other shell script #7020

Add shutdown.sh and match other shell script #7020

jgzl commented May 26, 2021

jgzl commented May 26, 2021

codecov bot commented May 26, 2021 •

edited

wu-sheng commented May 26, 2021

JaredTan95 commented May 26, 2021

kezhenxu94 May 26, 2021

jgzl May 26, 2021

kezhenxu94 May 26, 2021

dmsolr May 26, 2021

jgzl Jun 1, 2021 •

edited

innerpeacez left a comment

innerpeacez May 26, 2021

jgzl May 26, 2021

jgzl Jun 1, 2021 •

edited

innerpeacez Jun 2, 2021

kezhenxu94 May 27, 2021 •

edited

jgzl May 27, 2021

kezhenxu94 May 27, 2021

jgzl May 27, 2021 •

edited

jgzl Jun 1, 2021

jgzl Jun 1, 2021

wu-sheng commented May 31, 2021

jgzl commented Jun 1, 2021

kezhenxu94 commented Jun 1, 2021

wu-sheng commented Jun 24, 2021

Add shutdown.sh and match other shell script #7020

Add shutdown.sh and match other shell script #7020

Conversation

jgzl commented May 26, 2021

jgzl commented May 26, 2021

codecov bot commented May 26, 2021 • edited

Codecov Report

wu-sheng commented May 26, 2021

JaredTan95 commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgzl Jun 1, 2021 • edited

Choose a reason for hiding this comment

innerpeacez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgzl Jun 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kezhenxu94 May 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgzl May 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented May 31, 2021

jgzl commented Jun 1, 2021

kezhenxu94 commented Jun 1, 2021

wu-sheng commented Jun 24, 2021

codecov bot commented May 26, 2021 •

edited

jgzl Jun 1, 2021 •

edited

jgzl Jun 1, 2021 •

edited

kezhenxu94 May 27, 2021 •

edited

jgzl May 27, 2021 •

edited