search

cockroachdb / pebble

RocksDB/LevelDB inspired key-value database in Go
BSD 3-Clause "New" or "Revised" License
4.77k stars 442 forks source link

compaction: Add tests for untested compaction error paths #1844

Open itsbilal opened 2 years ago

itsbilal commented 2 years ago

Currently, there are two TODOs in compaction.go around untested code paths, both concerning errors from logAndApply:

https://github.com/cockroachdb/pebble/blob/a4f88b796aeb07156512564261dd32ecadc80de6/compaction.go#L1605

https://github.com/cockroachdb/pebble/blob/a4f88b796aeb07156512564261dd32ecadc80de6/compaction.go#L2086

Errors in logAndApply are possible (eg. errors during Manifest write or Manifest rotation), so we should be testing these error paths in compactions as well. This can be done by injecting an error on some Manifest writes (maybe during some metamorphic tests or in a unit test specifically around manifest error) and ensuring that Pebble continues to return consistent results and does not panic over it.

Jira issue: PEBBLE-166

nicktrav commented 2 years ago

Errors in logAndApply

I'll note that we made some recent changes around logAndApply in both #1794 and #1785. The latter added a test by doing what you mentioned. Perhaps we should have removed the TODO as part of that patch?