ArcticDB icon indicating copy to clipboard operation
ArcticDB copied to clipboard

Fix #1384 by refactoring `VersionMap` to include all combinations of `LoadType`s and whether to `include_deleted`

Open IvoDD opened this issue 11 months ago • 0 comments

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 a LoadStrategy which encapsulates the LoadType, include_deleted and the timestamp or version for LOAD_DOWNTO and LOAD_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?

IvoDD avatar Mar 20 '24 09:03 IvoDD