[GitHub] [bookkeeper] rdhabalia commented on a change in pull request #2303: QuorumCoverage should only count unknown nodes

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

rdhabalia commented on a change in pull request #2303: QuorumCoverage should only count unknown nodes
URL: https://github.com/apache/bookkeeper/pull/2303#discussion_r410841315
 
 

 ##########
 File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java
 ##########
 @@ -373,29 +373,43 @@ public synchronized void addBookie(int bookieIndexHeardFrom, int rc) {
         public synchronized boolean checkCovered() {
             // now check if there are any write quorums, with |ackQuorum| nodes available
             for (int i = 0; i < ensembleSize; i++) {
-                int nodesNotCovered = 0;
-                int nodesOkay = 0;
-                int nodesUninitialized = 0;
+                /* Nodes which have either responded with an error other than NoSuch{Entry,Ledger},
 
 Review comment:
   I think I am missing something here So, I have a question. While doing a ledger recovery, bk client waits for response from [(Qw - Qa) + 1 bookies](https://bookkeeper.apache.org/docs/4.10.0/development/protocol/)
   So, if we have ledger with `E=3, W=2, A=3` and if bk-client receives ack from one of the bookie then: Qw-Qa+1 = (2-2+1) = 1 >= Response (1).
   So, `checkCovered()` should return true.
   However, with this change it fails on such useacase:
   eg.
   ```
   RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(
           2, 2, 3);
   Set<Integer> resultSet = Sets.newHashSet(BKException.Code.OK,
           BKException.Code.UNINITIALIZED, BKException.Code.UNINITIALIZED);
   DistributionSchedule.QuorumCoverageSet covSet = schedule.getCoverageSet();
   int index =0;
   for (Integer i : resultSet) {
       covSet.addBookie(index++, i);
   }
   boolean covSetSays = covSet.checkCovered();
   assertTrue(covSetSays);
   ```
   
   So, can you please confirm the above assumption is correct or am I missing anything here?
   and can't we just check Qw-Qa+1 in this method:
   ```
   public synchronized boolean checkCovered() {
   int nodesUnknown = 0;
   for (int i = 0; i < covered.length; i++) {
       if (covered[i] != BKException.Code.OK
               && covered[i] != BKException.Code.NoSuchEntryException
               && covered[i] != BKException.Code.NoSuchLedgerExistsException) {
               nodesUnknown++;
           }
   }
   int expectedKnownNodes = (writeQuorumSize - ackQuorumSize) + 1;
   return (ensembleSize - nodesUnknown) >= expectedKnownNodes;
   }
   ```

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