Skip to content

Snapshot Deletions - #38

Open
gabrielbornstein wants to merge 10 commits into
mainfrom
gbornste/snapshot-deletions
Open

gabrielbornstein wants to merge 10 commits into
mainfrom
gbornste/snapshot-deletions

Conversation

@gabrielbornstein

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/turtle_kv/kv_store.cpp Resolved
Comment thread src/turtle_kv/packed_checkpoint.cpp Resolved
Comment thread src/turtle_kv/kv_store.hpp Resolved
Comment thread src/turtle_kv/kv_store.cpp Resolved
//
Status KVStore::delete_snapshot(EditOffset checkpoint_edit_offset) noexcept
{
// TODO: [Gabe Bornstein 10/12/26] commit_checkpoint does this. Do we need delete_snapshot to do

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could replace this with BATT_CHECK_OK(this->wait_for_recovery()) -- At the point when its possible for application code to call delete_snapshot, we know recovery is done because KVStore::open has returned.

Comment thread conanfile.py Resolved
Comment thread src/turtle_kv/checkpoint_generator.cpp Outdated
{
std::lock_guard<std::mutex> lock{this->checkpoint_mutex_};

if (this->active_checkpoints_.num_active_checkpoints == 0) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: your comment in PackedActiveCheckpoints::erase: these checks should move into that function, making this one as simple as 1. lock the mutex, 2. call erase.

Comment thread src/turtle_kv/kv_store.test.cpp Outdated
public:
void SetUp() override
{
KVStoreTest::SetUp();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this-> please

Comment thread src/turtle_kv/kv_store.test.cpp Outdated
//
{
Status status = kv_store->delete_snapshot(checkpoint_offsets.back());
EXPECT_FALSE(status.ok()) << "Deleting the newest checkpoint should fail";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specific code value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants