addBookieAndCheckCovered in RRQuorumCoverageSet doesn't work correctly if AcqQuorumSize is greater than (WriteQuorumSize+1)/2

[reddycharan]

QUESTION

Ensemble Size - 9
WriteQuorum Size - 7
AcqQuorum Size - 5

lets assume the following scenario

BK - means Bookie
BK0 (NonValidResponse) - BK1 (ValidResponse) -BK2 (NonValid) - BK3 (Valid) -BK4 (NonValid) - BK5 (Valid) -BK6 (NonValid) - BK7 (Valid) - BK8 (NonValid)

basically
False (0) - True (1) - False (2) - True (3) - False (4) - True (5) - False (6) - True (7) - False (8)

Here we got valid responses from only 4 bookies and also no writesets has more than ack number of valid responses, but RRQuorumCoverageSet's addBookieAndCheckCovered returns true in this case.

Before I confirm this as bug, I need others opinion on expectation of this RRQuorumCoverage.

Anyhow I added testcase to validate this logic
reddycharan@9eead22

[reddycharan]

@ivankelly @sijie can you please check if this is an issue with RRQuorumCoverageSet logic

[sijie]

@reddycharan nice catch. I think the logic there is a problematic. we should just make sure nodeOk is larger than or equal to ackQuorumSize.

[sijie]

@reddycharan actually I was thinking this for writes. but it turns out this is used for quorum read, if that's the case, I don't think the logic is wrong. Can you clarify your use case here?

[reddycharan]

As far as I understood, RRQuorumCoverageSet should return true for checkCovered only if all the writesets have atleast AckQuorum number of OKAY responses (in this OK/NoSuchEntryException/NoSuchLedgerExistsException).

If that is the correct expectation, then current implementation of RRQuorumCoverageSet doesn't work accordingly in the example I mentioned above.

[sijie]

RRQuorumCoverageSet should return true for checkCovered only if all the writesets have atleast AckQuorum number of OKAY responses

No. I don't think that is the meaning. The checkConvered here is used for quorum reads not for quorum writes, what you expect here is you should have at least one valid response from every quorum. so the check nodesNotCovered >= ackQuorumSize means if I have more "not covered" responses than ackQuorumSize, this quorum didn't see enough responses, the client should fail the quorum reads.

[ivankelly]

@reddycharan @sijie

Yes, it's as @sijie says. checkCovered is not for the write path as such. Not really for the read path either. It's for fencing.

The intuition behind it is, if a entry X has been acknowledged to the client, it has to have been written to a full ack quorum of bookies. Conversely, if at least one bookie in each possible ack quorum has acknowledged the a fence operation, then it is impossible for any entry to be acknowledged to the client from that point on.