ArcticDB
ArcticDB copied to clipboard
Fix #1384 by refactoring `VersionMap` to include all combinations of `LoadType`s and whether to `include_deleted`
Reference Issues/PRs
Fixes #1384
What does this implement or fix?
The #1384 bug also applies to as_of=<VERSION_ID> it is just not seen in the repro because it is not looking for an old enough version.
The bug is that we perform the check on TOMBSTONE_ALL only for LOAD_UNDELETED and LOAD_LATEST_UNDELETED. However LOAD_DOWNTO and LOAD_FROM_TIME should also only work on undeleted versions when querying with as_of.
As discussed we would like to have the ability to both load deleted and undeleted with LOAD_DOWNTO and LOAD_FROM_TIME. This essentially leaves us with the full cartesian product of possible load types and whether to include deleted or not.
This the majority of this change is to do the refactor to extract the include_deleted out of the load type and ensure correctness.
The PR is split in several commits, each one of which should be reviewed separately. The commits have detailed descriptions but in a few words the commits do the following:
- Introduce a failing cpp test to demonstrate #1384
- Moves entry updates when tombstoning inside
write_tombstone - Revamp version map caching logic to simplify the checks and to make less unnecessary cache misses
- Fix cache invalidation when tombstoning
- Refactor the
LoadTypeinto aLoadStrategywhich encapsulates theLoadType,include_deletedand the timestamp or version forLOAD_DOWNTOandLOAD_FROM_TIME. This fixes the #1384 bug and provides new functionality - Small refactor with renames to make the logic inside
follow_version_chainclearer. - Decrease the default
unsync_tolerancebecause previously we were ~always skipping the cache and deeming it outdated. Also fixes a bug where we don't clear the cache when clearing the storage.
Any other comments?
Checklist
Checklist for code changes...
- [ ] Have you updated the relevant docstrings, documentation and copyright notice?
- [ ] Is this contribution tested against all ArcticDB's features?
- [ ] Do all exceptions introduced raise appropriate error messages?
- [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?