[GitHub] [zookeeper] KimRasak opened a new pull request #1164: Fix haveDelivered wrong implementation.

[GitBox <gi...@apache.org>]

KimRasak opened a new pull request #1164: Fix haveDelivered wrong implementation.
URL: https://github.com/apache/zookeeper/pull/1164
 
 
   # Original implementation and mine
   - The original implementation of `QuorumCnxManager::haveDelivered` returns true once it finds a queue is empty, and returns false if all queues aren't empty.
   - My implementation returns false once it finds a queue not empty, and returns true if all queues are emtpy.
   
   # Reasons
   There are 2 reasons why I think the implementation of `QuorumCnxManager::haveDelivered` is wrong.
   ## 1. comment mismatch
   The comment of `QuorumCnxManager::haveDelivered` says:
   >  /**
        * Check if all queues are empty, indicating that all messages have been delivered.
        */
   As the comment says, the method should return true if **all** queues are empty, But the implementation returns true if one of the queues is empty.
   
   # 2. The methods functionality
   In `FastLeaderElection::lookForLeader`:
   ```
   if(manager.haveDelivered()){
       sendNotifications();
   } else {
       manager.connectAll();
   }
   ```
   I believe the code wants to do the following:
   - If The manager has sent all messages(but receives nothing),  send notifications again.
   - Otherwise, if some queue of `queueSendMap` isn't empty, the connection to that server must have broken down or haven't been established.
   Therefore, `manager.haveDelivered()` should check whether some queue isn't empty.
   (Note that `manager.connectAll`only **tries** connecting all servers. It skips the servers that "I"(the server itself) have connected to.)

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


With regards,
Apache Git Services