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=2` 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:
[email protected]
With regards,
Apache Git Services
