Make modification of snapshot atomic and corresponding tombstone handling changes #1650

phoebusm · 2024-06-26T11:20:32Z

Reference Issues/PRs

Originally only for forward porting https://mangit.maninvestments.com/projects/DATA/repos/arcticc/pull-requests/1095/overview
But corresponding test requires https://github.com/man-group/arcticdb-enterprise/issues/32 so part of changes is included too.

What does this implement or fix?

Modification of snapshot will not longer delete the original SNAPSHOT_REF key. It will be simply overwritten so atomicity can be achieved
As the changes will affect op logs as well and the test cannot be made without https://github.com/man-group/arcticdb-enterprise/issues/32 so some corresponding changes for that ticket as well. More details can be found there.

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?

-Supplymentary work for tombstone handling

phoebusm · 2024-06-26T15:08:25Z

Depended by https://github.com/man-group/arcticdb-enterprise/pull/133

poodlewars · 2024-07-01T16:31:43Z

This change requires a test in this repo. I think that I did something similar a while ago for vref atomicity - https://github.com/man-group/ArcticDB/blob/master/python/tests/stress/arcticdb/version_store/test_concurrent_read_and_write.py

poodlewars · 2024-07-01T16:12:39Z

cpp/arcticdb/async/tasks.hpp

+        typename=std::enable_if_t< // Make obvious if iterator& is captured
+                        is_same_cvref_removed<T, entity::VariantKey>::value
+                        || is_iter_of<T, entity::VariantKey>::value
+                        || is_pointer_of<T, entity::VariantKey>::value>>
 struct KeyExistsTask : BaseTask {


The old implementation was going to great lengths to ensure that we are never copying the key in to the KeyExistsTask, why do we need to relax that constraint now??

In the old implementation, the task itself doesn't hold the memory during construction. This could be a ticking timebomb (at least I hit it) as the memory could no longer be valid by the time the task is run.
IMO in the worker-pool model, every task should be self-sufficient - not depending on memory resides outside the object

This is still dangerous, it is allowing iterators into vectors that can go out of scope and raw pointers. To ensure liveness of the key memory, only accept rvalue references and shared pointers

I am happy to do so if there is no concern for performance. It can simplify the mad template parameter checking in this class.
I thought it is designed to accept iterator and shared pointer for a reason.
My changes is only for making the class store the key locally if a lvalue key is passed

Sorry, yes, copying lvalues is also acceptable from a correctness perspective. I haven't seen this level of madness in any similar methods though, how do they solve this problem? I've definitely seen overloaded constructors that copy lvalues or move rvalues, are there other approaches?

The template has been removed as KeyExistsTask now store copy of key, per discussed

poodlewars · 2024-07-01T16:20:03Z

cpp/arcticdb/version/local_versioned_engine.hpp

@@ -400,6 +394,7 @@ class LocalVersionedEngine : public VersionedEngine {
    }
    void _test_set_store(std::shared_ptr<Store> store);
    std::shared_ptr<VersionMap> _test_get_version_map();
+    SymbolList& _test_get_symbol_list();


Return by value to avoid confusion if this is just for testing?

For the testing only in the other repo

Agree with Alex, returning by reference is unnecessarily dangerous for test code

It is the last resort for https://github.com/man-group/arcticdb-enterprise/blob/2af2e32dc3321b28f03a9167bc0d40092f22e7a8/cpp/arcticdb-enterprise/background_delete/test/test_deletion.cpp#L74

It's for testing only so the risk won't get propagated to production code

Actually, why do we need this at all? Just call symbol_list() directly from the test?

The dependency is removed by turning off the symbol_list in the test

poodlewars · 2024-07-01T16:22:10Z

cpp/arcticdb/version/version_map_batch_methods.hpp

@@ -37,6 +37,16 @@ enum class BatchGetVersionOption {
    COUNT
 };

+inline std::optional<std::string> collect_futures_exceptions(auto futures) {


Move the futures in?

poodlewars · 2024-07-01T16:23:30Z

cpp/arcticdb/version/version_store_api.cpp

-        tombstone_snapshot(store(), to_ref(snap_key), std::move(snap_segment), version_map()->log_changes());
-    } else {
-        delete_trees_responsibly(deleted_keys, get_master_snapshots_map(store()), snap_name).get();
+    if(variant_key_type(snap_key) != KeyType::SNAPSHOT_REF || !cfg().write_options().delayed_deletes()) {


The arcticc version with bool is_delete_keys_immediately was clearer

Hmmm got it.... Let me re-align my style

The old is_delete_keys_immediately is kinda a relic of a sepearate implementation

poodlewars · 2024-07-01T16:28:14Z

cpp/arcticdb/version/version_store_api.cpp

-        tombstone_snapshot(store(), to_ref(snap_key), std::move(snap_segment), version_map()->log_changes());
-    } else {
-        delete_trees_responsibly(deleted_keys, get_master_snapshots_map(store()), snap_name).get();
+    if(variant_key_type(snap_key) != KeyType::SNAPSHOT_REF || !cfg().write_options().delayed_deletes()) {


I know it was here already, but this check,

variant_key_type(snap_key) != KeyType::SNAPSHOT_REF

is really confusing. Don't we already know what type snap_key is??

I had the same question when I was working on it. My guess is for backward compatibility with KeyType::SNAPSHOT:

ArcticDB/cpp/arcticdb/version/snapshot.cpp

Line 214 in 3adbed4

store->iterate_type(KeyType::SNAPSHOT, [&ret, &snap_name](VariantKey &&vk) {

cpp/arcticdb/version/version_store_api.cpp

cpp/arcticdb/version/local_versioned_engine.hpp

alexowens90 · 2024-07-04T09:45:54Z

cpp/arcticdb/version/local_versioned_engine.hpp

@@ -400,6 +394,7 @@ class LocalVersionedEngine : public VersionedEngine {
    }
    void _test_set_store(std::shared_ptr<Store> store);
    std::shared_ptr<VersionMap> _test_get_version_map();
+    SymbolList& _test_get_symbol_list();


Agree with Alex, returning by reference is unnecessarily dangerous for test code

alexowens90 · 2024-07-04T09:47:10Z

cpp/arcticdb/version/local_versioned_engine.cpp

@@ -895,11 +897,11 @@ folly::Future<folly::Unit> LocalVersionedEngine::delete_trees_responsibly(

    storage::ReadKeyOpts read_opts;
    read_opts.ignores_missing_key_ = true;
-    auto data_keys_to_be_deleted = get_data_keys_set(store(), *keys_to_delete, read_opts);
+    auto data_keys_to_be_deleted = get_data_keys_set(store, *keys_to_delete, read_opts);


I've renamed this function in #1657, whoever merges second will need to rebase

I'll rebase it once the PR is approved

cpp/arcticdb/async/tasks.hpp

alexowens90 · 2024-07-04T09:58:33Z

cpp/arcticdb/async/tasks.hpp

+        typename=std::enable_if_t< // Make obvious if iterator& is captured
+                        is_same_cvref_removed<T, entity::VariantKey>::value
+                        || is_iter_of<T, entity::VariantKey>::value
+                        || is_pointer_of<T, entity::VariantKey>::value>>
 struct KeyExistsTask : BaseTask {


This is still dangerous, it is allowing iterators into vectors that can go out of scope and raw pointers. To ensure liveness of the key memory, only accept rvalue references and shared pointers

-Make modification of snapshot atomic

ad57e51

-Supplymentary work for tombstone handling

phoebusm requested review from alexowens90, willdealtry and poodlewars as code owners June 26, 2024 11:20

phoebusm changed the title ~~-Make modification of snapshot atomic~~ Make modification of snapshot atomic and corresponding tombstone handling changes Jun 26, 2024

phoebusm added 2 commits July 1, 2024 10:32

Make delete_trees_responsibly a free function

fb9a28f

Minor amendment

3adbed4

poodlewars requested changes Jul 1, 2024

View reviewed changes

phoebusm force-pushed the fix/make_mod_snapshot_atomic branch from 5021c50 to 1ef07c7 Compare July 2, 2024 09:09

Address PR comment

bc0bc7c

phoebusm force-pushed the fix/make_mod_snapshot_atomic branch from 0f803d3 to bc0bc7c Compare July 2, 2024 09:15

Add local atomicity test

9b68c33

alexowens90 requested changes Jul 4, 2024

View reviewed changes

Address PR comment

4ac073c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make modification of snapshot atomic and corresponding tombstone handling changes #1650

Make modification of snapshot atomic and corresponding tombstone handling changes #1650

phoebusm commented Jun 26, 2024 •

edited

Loading

phoebusm commented Jun 26, 2024

poodlewars commented Jul 1, 2024

poodlewars Jul 1, 2024

phoebusm Jul 1, 2024 •

edited

Loading

alexowens90 Jul 4, 2024

phoebusm Jul 4, 2024

alexowens90 Jul 4, 2024

phoebusm Jul 4, 2024

poodlewars Jul 1, 2024

phoebusm Jul 1, 2024

alexowens90 Jul 4, 2024

phoebusm Jul 4, 2024 •

edited

Loading

alexowens90 Jul 4, 2024

phoebusm Jul 4, 2024

poodlewars Jul 1, 2024

poodlewars Jul 1, 2024

phoebusm Jul 1, 2024

phoebusm Jul 1, 2024

poodlewars Jul 1, 2024

phoebusm Jul 1, 2024

alexowens90 Jul 4, 2024

alexowens90 Jul 4, 2024

phoebusm Jul 4, 2024

alexowens90 Jul 4, 2024

Make modification of snapshot atomic and corresponding tombstone handling changes #1650

Are you sure you want to change the base?

Make modification of snapshot atomic and corresponding tombstone handling changes #1650

Conversation

phoebusm commented Jun 26, 2024 • edited Loading

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

phoebusm commented Jun 26, 2024

poodlewars commented Jul 1, 2024

Choose a reason for hiding this comment

phoebusm Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoebusm Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoebusm commented Jun 26, 2024 •

edited

Loading

phoebusm Jul 1, 2024 •

edited

Loading

phoebusm Jul 4, 2024 •

edited

Loading