ztzg commented on a change in pull request #1054: ZOOKEEPER-1112: Add support 
for C client for SASL authentication
URL: https://github.com/apache/zookeeper/pull/1054#discussion_r314208678
 
 

 ##########
 File path: bin/zkServer.sh
 ##########
 @@ -212,6 +212,7 @@ stop)
     else
       $KILL $(cat "$ZOOPIDFILE")
       rm "$ZOOPIDFILE"
+      sleep 1
 
 Review comment:
   Hi Enrico,
   
   Probably for the same reason that there is a `sleep 3` in `restart`.  Here 
is what I have put in the corresponding commit message:
   
   ---
   
   ZOOKEEPER-1112: Make `zkServer.sh stop` more reliable
   
   Kill is asynchronous, and without the sleep, the server's TCP port can still 
be busy when the next server is started—causing flaky runs of the C client's 
test suite.
   
   (It would probably be better to spin a few times, probing with `ps -p`.)
   
   ---
   
   This is not hypothetical: I am observing `FAILED TO START` errors in the C 
test suite; the log consistently shows that those are caused by 
`java.net.BindException: Address already in use`.
   
   As noted above, the `sleep` is far from optimal, an adaptive mechanism would 
be better—but I did not want to make the patch too complicated.
   
   Here is what I would suggest: I will drop the patch from my upcoming respin, 
create a dedicated ticket, and carry it locally in the meantime.

----------------------------------------------------------------
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:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to