Fail point-in-time WAL recovery upon IOError reading WAL

[riversand963]

If options.wal_recovery_mode == WALRecoveryMode::kPointInTimeRecovery, RocksDB stops replaying WAL once hitting an error and discards the rest of the WAL. This can lead to data loss if the error occurs at an offset smaller than the last sync'ed offset.
Ideally, RocksDB point-in-time recovery should permit recovery if the error occurs after last synced offset while fail recovery if error occurs before the last synced offset. However, RocksDB does not track the synced offset of WALs. Consequently, RocksDB does not know whether an error occurs before or after the last synced offset. An error can be one of the following.

• WAL record checksum mismatch. This can result from both corruption of synced data and dropping of unsynced data during shutdown. We cannot be sure which one. In order not to defeat the original motivation to permit the latter case, we keep the original behavior of point-in-time WAL recovery.
• IOError. This means the WAL can be bad, an indicator of whole file becoming unavailable, not to mention synced part of the WAL. Therefore, we choose to modify the behavior of point-in-time recovery and fail the database recovery.

Test plan (devserver):
make check

[ajkr]

LGTM!

I wonder if we should reject Open() for all errors returned by the underlying Env. It's difficult right now because we can't distinguish where status->IsCorruption() originates -- it could either be from the underlying Env or from RocksDB detecting checksum mismatch. In any case I think this PR is a step forward and should be landed.

[riversand963]

LGTM!

I wonder if we should reject Open() for all errors returned by the underlying Env. It's difficult right now because we can't distinguish where status->IsCorruption() originates -- it could either be from the underlying Env or from RocksDB detecting checksum mismatch. In any case I think this PR is a step forward and should be landed.

In other places such as https://github.com/facebook/rocksdb/blob/master/db/version_set.h#L1162, RocksDB uses a separate status variable to keep track of IO errors. We probably can follow similar approach by adding a member variable to log::Reader::Reporter.

[facebook-github-bot]

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@riversand963 merged this pull request in 717749f.

[yiwu-arbug]

Thanks for the improvement!