[GitHub] [bookkeeper] dlg99 commented on issue #2612: Wrong ReadLastAddConfirmed logic that can lead to data loss in client applications

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

dlg99 commented on issue #2612:
URL: https://github.com/apache/bookkeeper/issues/2612#issuecomment-781608819


   1. Looks like bookie recovery process after the restart should be extended. IIRC it processes journal to replay entries that didn't make it to the entry log. It probably should check ledgers metadata against local data to detect data loss and go into a recovery state to read missing data from other bookies (or quarantine itself somehow and let autoreplicator deal with this)
   In this case bookie could return something like `rc:MISSINGDATA` instead of `rc:ENOENTRY` for incomplete ledgers.
   
   2. alternatively, in addition to
   ```
           if (heardValidResponse
               && coverageSet.checkCovered()
               && !completed) {
   ```
   we can add check for ` && (maxRecoveredData.getLastAddConfirmed() != LedgerHandle.INVALID_ENTRY_ID || numResponsesPending == 0)`. Going with `numResponsesPending == 0` alone will increase request latency.
   
   The problem, as I understood it, is that maxRecoveredData didn't get updated and the read LAC op still decided to not wait for other responses.
   


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