Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused track_read_versions #6871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 18 additions & 32 deletions src/kv/apply_changes.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ namespace ccf::kv
// sets to their underlying Maps. Calls f() at most once, iff the writes are
// applied, to retrieve a unique Version for the write set and return the max
// version which can have a conflict with the transaction.
//
// The track_read_versions parameter tells the store if it needs to track the
// last read version for every key. This is required for backup execution as
// described at the top of tx.h

using VersionLastNewMap = Version;
using VersionResolver = std::function<std::tuple<Version, VersionLastNewMap>(
Expand All @@ -40,7 +36,6 @@ namespace ccf::kv
ccf::kv::ConsensusHookPtrs& hooks,
const MapCollection& new_maps,
const std::optional<Version>& new_maps_conflict_version,
bool track_read_versions,
bool track_deletes_on_missing_keys,
const std::optional<Version>& expected_rollback_count = std::nullopt)
{
Expand Down Expand Up @@ -88,7 +83,7 @@ namespace ccf::kv
{
for (auto it = views.begin(); it != views.end(); ++it)
{
if (!it->second->prepare(track_read_versions))
if (!it->second->prepare())
{
ok = false;
break;
Expand Down Expand Up @@ -132,40 +127,31 @@ namespace ccf::kv
std::tie(version, version_last_new_map) =
version_resolver_fn(!new_maps.empty());

if (!track_read_versions)
// Transfer ownership of these new maps to their target stores, iff we
// have writes to them
for (const auto& [map_name, map_ptr] : new_maps)
{
// Transfer ownership of these new maps to their target stores, iff we
// have writes to them
for (const auto& [map_name, map_ptr] : new_maps)
const auto it = views.find(map_name);
if (it != views.end() && it->second->has_writes())
{
const auto it = views.find(map_name);
if (it != views.end() && it->second->has_writes())
{
map_ptr->get_store()->add_dynamic_map(version, map_ptr);
}
map_ptr->get_store()->add_dynamic_map(version, map_ptr);
}
}

for (auto it = views.begin(); it != views.end(); ++it)
{
it->second->commit(
version, track_read_versions, track_deletes_on_missing_keys);
}
for (auto it = views.begin(); it != views.end(); ++it)
{
it->second->commit(version, track_deletes_on_missing_keys);
}

// Collect ConsensusHooks
for (auto it = views.begin(); it != views.end(); ++it)
// Collect ConsensusHooks
for (auto it = views.begin(); it != views.end(); ++it)
{
auto hook_ptr = it->second->post_commit();
if (hook_ptr != nullptr)
{
auto hook_ptr = it->second->post_commit();
if (hook_ptr != nullptr)
{
hooks.push_back(std::move(hook_ptr));
}
hooks.push_back(std::move(hook_ptr));
}
}
else
{
// A linearizability violation was detected
ok = false;
}
}

for (auto it = changes.begin(); it != changes.end(); ++it)
Expand Down
4 changes: 0 additions & 4 deletions src/kv/committable_tx.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ namespace ccf::kv
*/
CommitResult commit(
const ccf::ClaimsDigest& claims = ccf::empty_claims(),
bool track_read_versions = false,
std::function<std::tuple<Version, Version>(bool has_new_map)>
version_resolver = nullptr,
std::function<void(
Expand Down Expand Up @@ -170,7 +169,6 @@ namespace ccf::kv
hooks,
pimpl->created_maps,
new_maps_conflict_version,
track_read_versions,
track_deletes_on_missing_keys);

if (maps_created)
Expand Down Expand Up @@ -422,15 +420,13 @@ namespace ccf::kv
throw std::logic_error("Reserved transaction cannot be empty");

std::vector<ConsensusHookPtr> hooks;
bool track_read_versions = false;
bool track_deletes_on_missing_keys = false;
auto c = apply_changes(
all_changes,
[this](bool) { return std::make_tuple(version, version - 1); },
hooks,
pimpl->created_maps,
version,
track_read_versions,
track_deletes_on_missing_keys,
rollback_count);
success = c.has_value();
Expand Down
7 changes: 2 additions & 5 deletions src/kv/kv_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -599,11 +599,8 @@ namespace ccf::kv
virtual ~AbstractCommitter() = default;

virtual bool has_writes() = 0;
virtual bool prepare(bool track_commits) = 0;
virtual void commit(
Version v,
bool track_read_versions,
bool track_deletes_on_missing_keys) = 0;
virtual bool prepare() = 0;
virtual void commit(Version v, bool track_deletes_on_missing_keys) = 0;
virtual ConsensusHookPtr post_commit() = 0;
};

Expand Down
43 changes: 8 additions & 35 deletions src/kv/untyped_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ namespace ccf::kv::untyped
return committed_writes || change_set.has_writes();
}

bool prepare(bool track_read_versions) override
bool prepare() override
{
auto& map_roll = map.get_roll();

Expand Down Expand Up @@ -185,9 +185,7 @@ namespace ccf::kv::untyped
// conflicts then ensure that the read versions also match.
if (
!search.has_value() ||
std::get<0>(it->second) != search.value().version ||
(track_read_versions &&
std::get<1>(it->second) != search.value().read_version))
std::get<0>(it->second) != search.value().version)
{
LOG_DEBUG_FMT("Read depends on invalid version of entry");
return false;
Expand All @@ -198,12 +196,9 @@ namespace ccf::kv::untyped
return true;
}

void commit(
Version v,
bool track_read_versions,
bool track_deletes_on_missing_keys) override
void commit(Version v, bool track_deletes_on_missing_keys) override
{
if (change_set.writes.empty() && !track_read_versions)
if (change_set.writes.empty())
{
commit_version = change_set.start_version;
return;
Expand All @@ -212,30 +207,6 @@ namespace ccf::kv::untyped
auto& map_roll = map.get_roll();
auto state = map_roll.commits->get_tail()->state;

// To track conflicts the read version of all keys that are read or
// written within a transaction must be updated.
if (track_read_versions)
{
for (auto it = change_set.reads.begin(); it != change_set.reads.end();
++it)
{
auto search = state.get(it->first);
if (!search.has_value())
{
continue;
}
state =
state.put(it->first, VersionV{search->version, v, search->value});
}
if (change_set.writes.empty())
{
commit_version = change_set.start_version;
map.roll.commits->insert_back(map.roll.create_new_local_commit(
commit_version, std::move(state), change_set.writes));
return;
}
}

// Record our commit time.
commit_version = v;
committed_writes = true;
Expand Down Expand Up @@ -439,14 +410,16 @@ namespace ccf::kv::untyped
return true;
}

bool prepare(bool) override
bool prepare() override
{
// Snapshots never conflict
return true;
}

void commit(Version, bool, bool) override
void commit(Version v, bool track_deletes_on_missing_keys) override
{
(void)v;
(void)track_deletes_on_missing_keys;
// Version argument is ignored. The version of the roll after the
// snapshot is applied depends on the version of the map at which the
// snapshot was taken.
Expand Down
2 changes: 1 addition & 1 deletion src/node/rpc/frontend.h
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ namespace ccf
// else args owns a valid Tx relating to a non-pending response, which
// should be applied
ccf::kv::CommittableTx& tx = *args.owned_tx;
ccf::kv::CommitResult result = tx.commit(ctx->claims, false);
ccf::kv::CommitResult result = tx.commit(ctx->claims);

switch (result)
{
Expand Down
3 changes: 1 addition & 2 deletions src/node/snapshotter.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ namespace ccf
pending_snapshots[generation_count] = {};
pending_snapshots[generation_count].version = snapshot_version;

auto rc =
tx.commit(cd, false, nullptr, capture_ws_digest_and_commit_evidence);
auto rc = tx.commit(cd, nullptr, capture_ws_digest_and_commit_evidence);
if (rc != ccf::kv::CommitResult::SUCCESS)
{
LOG_FAIL_FMT(
Expand Down
Loading