You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/11/12 12:55:30 UTC

[GitHub] [apisix-dashboard] starsz opened a new pull request #796: feat: install signal handler for graceful shutdown (#737)

starsz opened a new pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [ ] Bugfix
   - [x] New feature provided
   - [ ] Improve performance
   
   - Related issues
   
   Issue: https://github.com/apache/apisix-dashboard/issues/737
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-726475419


   Hi, @nic-chen , I had added a test case. 
   Hi, @juzhiyuan, please have a look. Am I doing this right?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r523689423



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,29 +47,47 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
+check_logfile
+
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
+    echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
-    echo "failed: should not write info log when level is warn"
+clean_logfile
+
+# change level and test signal
+
+sed -i 's/warn/info/' conf/conf.yaml
+
+./manager-api &>/dev/null &
+sleep 3
+pkill -2 -f manager-api
+sleep 6
+
+check_logfile
+
+if [[ `ps -ef | grep "[m]anager-api" -c` -eq '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
     exit 1
 fi
 
-#change level and path
+if [[ `grep -c "server receive interrupt" ${logfile}` -ne '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
+    exit 1
+fi
+
+clean_logfile
+
+#change path
 
 sed -i 's/file_path: logs\/error.log/file_path: .\/error.log/' conf/conf.yaml
-sed -i 's/warn/info/' conf/conf.yaml

Review comment:
       why do we need to delete this line? I think it should be useful for old test case.
   
   one PR for one thing, please focus on `install signal handler for graceful shutdown`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-727582808


   > need to rebase your branch @starsz
   
   OK. Rebased.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-726498071


   Yes! clever you are!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r522871153



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,31 +47,49 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
-    exit 1
-fi
+check_logfile
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
     echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-#change level and path
+clean_logfile
+
+#change level
 
-sed -i 's/file_path: logs\/error.log/file_path: .\/error.log/' conf/conf.yaml

Review comment:
       please don't delete the existing case，it is for testing log filepath.thanks




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-726064119


   OK. No problem.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on a change in pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r523392210



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,31 +47,49 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
-    exit 1
-fi
+check_logfile
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
     echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-#change level and path
+clean_logfile
+
+#change level
 
-sed -i 's/file_path: logs\/error.log/file_path: .\/error.log/' conf/conf.yaml

Review comment:
       OK.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on a change in pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r523765072



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,29 +47,47 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
+check_logfile
+
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
+    echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
-    echo "failed: should not write info log when level is warn"
+clean_logfile
+
+# change level and test signal
+
+sed -i 's/warn/info/' conf/conf.yaml
+
+./manager-api &>/dev/null &
+sleep 3
+pkill -2 -f manager-api
+sleep 6
+
+check_logfile
+
+if [[ `ps -ef | grep "[m]anager-api" -c` -eq '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
     exit 1
 fi
 
-#change level and path
+if [[ `grep -c "server receive interrupt" ${logfile}` -ne '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
+    exit 1
+fi
+
+clean_logfile
+
+#change path
 
 sed -i 's/file_path: logs\/error.log/file_path: .\/error.log/' conf/conf.yaml
-sed -i 's/warn/info/' conf/conf.yaml

Review comment:
       Hi, I didn't delete this line. This line just is moved to the front.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-727350426


   need to rebase your branch @starsz 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] juzhiyuan merged pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
juzhiyuan merged pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on a change in pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r526178607



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,29 +47,47 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
+check_logfile
+
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
+    echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
-    echo "failed: should not write info log when level is warn"
+clean_logfile
+
+# change level and test signal
+
+sed -i 's/warn/info/' conf/conf.yaml

Review comment:
       That's right. Agree with you.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-726062566


   need test case.
   please refer: 
   https://github.com/apache/apisix-dashboard/blob/v2.0/api/test/shell/cli_test.sh


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] juzhiyuan commented on pull request #796: feat: install signal handler for graceful shutdown (#737)

Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#issuecomment-726108205


   ![image](https://user-images.githubusercontent.com/2106987/98951544-5a18c800-2535-11eb-83dd-18961abd0e7d.png)
   
   Hi, welcome to use the GitHub issue keywords to automatically close the target issue once this PR is merged.
   
   examples:
   1. close #9999
   2. resolve #9999
   3. fix #9999


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #796: feat: install signal handler for graceful shutdown

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #796:
URL: https://github.com/apache/apisix-dashboard/pull/796#discussion_r525196229



##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,29 +47,47 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
+check_logfile
+
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
+    echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
-    echo "failed: should not write info log when level is warn"
+clean_logfile
+
+# change level and test signal
+
+sed -i 's/warn/info/' conf/conf.yaml

Review comment:
       I think that is unsafe, should match and replace `level: warn`

##########
File path: api/test/shell/cli_test.sh
##########
@@ -34,29 +47,47 @@ go build -o ./manager-api .
 sleep 3
 pkill -f manager-api
 
-if [[ ! -f "./logs/error.log" ]]; then
-    echo "failed: failed to write log"
+check_logfile
+
+if [[ `grep -c "INFO" ${logfile}` -ne '0' ]]; then
+    echo "failed: should not write info log when level is warn"
     exit 1
 fi
 
-if [[ `grep -c "INFO" ./logs/error.log` -ne '0' ]]; then
-    echo "failed: should not write info log when level is warn"
+clean_logfile
+
+# change level and test signal
+
+sed -i 's/warn/info/' conf/conf.yaml
+
+./manager-api &>/dev/null &
+sleep 3
+pkill -2 -f manager-api
+sleep 6
+
+check_logfile
+
+if [[ `ps -ef | grep "[m]anager-api" -c` -eq '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
     exit 1
 fi
 
-#change level and path
+if [[ `grep -c "server receive interrupt" ${logfile}` -ne '1' ]]; then
+    echo "failed: the manager server didn't deal with signal in correct way"
+    exit 1
+fi
+
+clean_logfile
+
+#change path
 
 sed -i 's/file_path: logs\/error.log/file_path: .\/error.log/' conf/conf.yaml
-sed -i 's/warn/info/' conf/conf.yaml

Review comment:
       please check your commit log or git operation. 
   
   Your PR cannot contain any other commit that do not belong to you.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org