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

Allow to get the latest sequence number when creating the backup #1987

Merged
merged 4 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
4 changes: 2 additions & 2 deletions src/storage/storage.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ Status Storage::Open(DBOpenMode mode) {
return Status::OK();
}

Status Storage::CreateBackup() {
Status Storage::CreateBackup(uint64_t *sequence_number) {
LOG(INFO) << "[storage] Start to create new backup";
std::lock_guard<std::mutex> lg(config_->backup_mu);
std::string task_backup_dir = config_->GetBackupDir();
Expand All @@ -363,7 +363,7 @@ Status Storage::CreateBackup() {
}

std::unique_ptr<rocksdb::Checkpoint> checkpoint_guard(checkpoint);
s = checkpoint->CreateCheckpoint(tmpdir, config_->rocks_db.write_buffer_size * MiB);
s = checkpoint->CreateCheckpoint(tmpdir, config_->rocks_db.write_buffer_size * MiB, sequence_number);
Copy link
Member

Choose a reason for hiding this comment

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

Just a question:

  // sequence_number_ptr: if it is not nullptr, the value it points to will be
  // set to a sequence number guaranteed to be part of the DB, not necessarily
  // the latest. The default value of this parameter is nullptr.

From rocksdb, seems this might a bit less than backup? Or I misunderstanding this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, I didn't investigate when it would be less than the DB.

Copy link
Member

Choose a reason for hiding this comment

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

It was introduced here: facebook/rocksdb@3ffb3ba

Maybe we should notice user to pay-attention to that, replaying kv might not harm

Copy link
Member Author

Choose a reason for hiding this comment

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

@mapleFU I have updated the comment to address this, please take a look again.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have confirmed the implementation, it is the latest sequence number while starting to create the checkpoint, so it should be less the current backup DB if has any writes during this process.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I guess maybe it's not atomic between:

  *sequence_number = db_->GetLatestSequenceNumber();

  LiveFilesStorageInfoOptions opts;
  opts.include_checksum_info = get_live_table_checksum;
  opts.wal_size_for_flush = log_size_for_flush;

  std::vector<LiveFileStorageInfo> infos;
  {
    Status s = db_->GetLiveFilesStorageInfo(opts, &infos);

if (!s.ok()) {
LOG(WARNING) << "Failed to create checkpoint (snapshot) for backup. Error: " << s.ToString();
return {Status::DBBackupErr, s.ToString()};
Expand Down
2 changes: 1 addition & 1 deletion src/storage/storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class Storage {
Status SetOptionForAllColumnFamilies(const std::string &key, const std::string &value);
Status SetDBOption(const std::string &key, const std::string &value);
Status CreateColumnFamilies(const rocksdb::Options &options);
Status CreateBackup();
Status CreateBackup(uint64_t *sequence_number = nullptr);
git-hulk marked this conversation as resolved.
Show resolved Hide resolved
PragmaTwice marked this conversation as resolved.
Show resolved Hide resolved
void DestroyBackup();
Status RestoreFromBackup();
Status RestoreFromCheckpoint();
Expand Down
57 changes: 57 additions & 0 deletions tests/cppunit/storage_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*
*/

#include <__filesystem/operations.h>
git-hulk marked this conversation as resolved.
Show resolved Hide resolved
#include <config/config.h>
#include <gtest/gtest.h>
#include <status.h>
#include <storage/storage.h>

TEST(Storage, CreateBackup) {
std::error_code ec;

Config config;
config.db_dir = "test_backup_dir";
config.slot_id_encoded = false;

std::filesystem::remove_all(config.db_dir, ec);
assert(!ec);

auto storage = std::make_unique<engine::Storage>(&config);
auto s = storage->Open();
assert(s.IsOK());

constexpr int cnt = 10;
for (int i = 0; i < cnt; i++) {
rocksdb::WriteBatch batch;
batch.Put("k", "v");
assert(storage->Write(rocksdb::WriteOptions(), &batch).ok());
}
uint64_t sequence_number = 0;
s = storage->CreateBackup(&sequence_number);
assert(s.IsOK());
git-hulk marked this conversation as resolved.
Show resolved Hide resolved
assert(cnt == sequence_number);
// check if backup success without caring about the sequence number
s = storage->CreateBackup();
assert(s.IsOK());

std::filesystem::remove_all(config.db_dir, ec);
assert(!ec);
}
Loading