From ae70971dbf8023d91f911f7829e776e38675a15a Mon Sep 17 00:00:00 2001 From: Ivo Dilov Date: Wed, 20 Mar 2024 10:53:50 +0200 Subject: [PATCH] Fixes #1384 by checking for tombstone_all on LOAD_FROM_TIME and LOAD_DOWNTO WORK IN PROGRESS - previously we did the check for tombstone_all only on LOAD_UNDELETED and LOAD_LATEST_UNDELETED - Renamed the functions in version_utils which we use to check when to stop following the version chain to less confusing names - Google test VersionMap.FollowingVersionChainEndEarly now passes --- cpp/arcticdb/version/version_map.hpp | 8 ++-- cpp/arcticdb/version/version_map_entry.hpp | 1 + cpp/arcticdb/version/version_utils.hpp | 55 ++++++++++++---------- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/cpp/arcticdb/version/version_map.hpp b/cpp/arcticdb/version/version_map.hpp index d18a8f51a25..2fc8397bdab 100644 --- a/cpp/arcticdb/version/version_map.hpp +++ b/cpp/arcticdb/version/version_map.hpp @@ -170,10 +170,10 @@ class VersionMapImpl { next_key = read_segment_with_keys(seg, entry, load_progress); set_latest_version(entry, latest_version); } while (next_key - && !loaded_until_version_id(load_strategy, load_progress, latest_version) - && !loaded_until_timestamp(load_strategy, load_progress) - && load_latest_ongoing(load_strategy, entry) - && looking_for_undeleted(load_strategy, entry, load_progress)); + && continue_when_loading_version(load_strategy, load_progress, latest_version) + && continue_when_loading_from_time(load_strategy, load_progress) + && continue_when_loading_latest(load_strategy, entry) + && continue_when_loading_undeleted(load_strategy, entry, load_progress)); } entry->loaded_with_progress_ = load_progress; } diff --git a/cpp/arcticdb/version/version_map_entry.hpp b/cpp/arcticdb/version/version_map_entry.hpp index b1f03b5e503..ffb9c1f01d6 100644 --- a/cpp/arcticdb/version/version_map_entry.hpp +++ b/cpp/arcticdb/version/version_map_entry.hpp @@ -33,6 +33,7 @@ inline constexpr bool is_partial_load_type(LoadType load_type) { return load_type == LoadType::LOAD_DOWNTO || load_type == LoadType::LOAD_FROM_TIME; } + enum class VersionStatus { LIVE, TOMBSTONED, diff --git a/cpp/arcticdb/version/version_utils.hpp b/cpp/arcticdb/version/version_utils.hpp index 8706a4ca9f9..abf8a896fb8 100644 --- a/cpp/arcticdb/version/version_utils.hpp +++ b/cpp/arcticdb/version/version_utils.hpp @@ -231,22 +231,27 @@ inline bool is_positive_version_query(const LoadStrategy& load_strategy) { return load_strategy.load_until_version_.value() >= 0; } -inline bool loaded_until_version_id(const LoadStrategy &load_strategy, const LoadProgress& load_progress, const std::optional& latest_version) { +inline bool continue_when_loading_version(const LoadStrategy& load_strategy, const LoadProgress& load_progress, const std::optional& latest_version) { if (!load_strategy.load_until_version_) - return false; + // Should continue when not loading down to a version + return true; if (is_positive_version_query(load_strategy)) { if (load_progress.oldest_loaded_index_version_ > static_cast(*load_strategy.load_until_version_)) { - return false; + // Should continue when version was not reached + return true; } } else { if (latest_version.has_value()) { if (auto opt_version_id = get_version_id_negative_index(*latest_version, *load_strategy.load_until_version_); opt_version_id && load_progress.oldest_loaded_index_version_ > *opt_version_id) { - return false; + // Should continue when version was not reached + return true; } } else { - return false; + // TODO: Think about this in the context of slack discussion + // Should continue when latest undeleted version was not reached + return true; } } ARCTICDB_DEBUG(log::version(), @@ -255,7 +260,7 @@ inline bool loaded_until_version_id(const LoadStrategy &load_strategy, const Loa *load_strategy.load_until_version_, latest_version.value() ); - return true; + return false; } inline void set_latest_version(const std::shared_ptr& entry, std::optional& latest_version) { @@ -270,51 +275,49 @@ static constexpr timestamp nanos_to_seconds(timestamp nanos) { return nanos / timestamp(10000000000); } -inline bool loaded_until_timestamp(const LoadStrategy &load_strategy, const LoadProgress& load_progress) { +inline bool continue_when_loading_from_time(const LoadStrategy &load_strategy, const LoadProgress& load_progress) { if (!load_strategy.load_from_time_) - return false; + return true; // TODO: Add tests for new behavior here! auto loaded_deleted_or_undeleted_timestamp = load_strategy.include_deleted_ ? load_progress.earliest_loaded_timestamp_ : load_progress.earliest_loaded_undeleted_timestamp_; if (loaded_deleted_or_undeleted_timestamp > *load_strategy.load_from_time_) - return false; + return true; ARCTICDB_DEBUG(log::version(), "Exiting load from timestamp because request {} <= {}", - *load_strategy.load_from_time_, - loaded_deleted_or_undeleted_timestamp); - return true; + loaded_deleted_or_undeleted_timestamp, + *load_strategy.load_from_time_); + return false; } -inline bool load_latest_ongoing(const LoadStrategy &load_strategy, const std::shared_ptr &entry) { +inline bool continue_when_loading_latest(const LoadStrategy& load_strategy, const std::shared_ptr &entry) { if (!(load_strategy.load_type_ == LoadType::LOAD_LATEST && entry->get_first_index(load_strategy.include_deleted_).first)) return true; - ARCTICDB_DEBUG(log::version(), "Exiting because we found a non-deleted index in load latest"); + ARCTICDB_DEBUG(log::version(), "Exiting because we found the latest version with include_deleted: {}", load_strategy.include_deleted); return false; } -inline bool looking_for_undeleted(const LoadStrategy& load_strategy, const std::shared_ptr& entry, const LoadProgress& load_progress) { - if(load_strategy.include_deleted_) { +inline bool continue_when_loading_undeleted(const LoadStrategy& load_strategy, const std::shared_ptr& entry, const LoadProgress& load_progress) { + if (load_strategy.include_deleted_){ return true; } if(entry->tombstone_all_) { - const bool is_deleted_by_tombstone_all = entry->tombstone_all_->version_id() >= load_progress.oldest_loaded_index_version_; - if(is_deleted_by_tombstone_all) { + const bool is_deleted_by_tombstone_all = + entry->tombstone_all_->version_id() >= load_progress.oldest_loaded_index_version_; + if (is_deleted_by_tombstone_all) { ARCTICDB_DEBUG( - log::version(), - "Exiting because tombstone all key deletes all versions beyond: {} and the oldest loaded index has version: {}", - entry->tombstone_all_->version_id(), - load_progress.oldest_loaded_index_version_); + log::version(), + "Exiting because tombstone all key deletes all versions beyond: {} and the oldest loaded index has version: {}", + entry->tombstone_all_->version_id(), + load_progress.oldest_loaded_index_version_); return false; - } else { - return true; } - } else { - return true; } + return true; } inline bool penultimate_key_contains_required_version_id(const AtomKey& key, const LoadStrategy& load_strategy) {