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
LoadType
into aLoadStrategy
which encapsulates theLoadType
,include_deleted
and the timestamp or version forLOAD_DOWNTO
andLOAD_FROM_TIME
. This fixes the #1384 bug and provides new functionality - Small refactor with renames to make the logic inside
follow_version_chain
clearer. - Decrease the default
unsync_tolerance
because 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?