Skip to content
This repository was archived by the owner on Mar 1, 2026. It is now read-only.

Apply sst file manager in myrocks clone to add slow-rm during checkpoints removal #1386

Open
sunshine-Chun wants to merge 2 commits into
base: fb-mysql-8.0.28
Choose a base branch
from
Open

Apply sst file manager in myrocks clone to add slow-rm during checkpoints removal #1386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0378711
Apply sst file manager in myrocks clone to add slow-rm during checkpo…
sunshine-Chun Oct 27, 2023
23c5788
Address comments
sunshine-Chun Nov 6, 2023
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
28 changes: 24 additions & 4 deletions storage/rocksdb/clone/donor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ class [[nodiscard]] rdb_checkpoint final {
// Returns MySQL error code
[[nodiscard]] int init() {
assert(!m_active);
const auto result = myrocks::rocksdb_create_checkpoint(m_dir.c_str());
m_full_dir = make_dir_full_name(
Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated
m_dir.c_str(), m_next_sub_id.fetch_add(1, std::memory_order_relaxed));
Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated
const auto result = myrocks::rocksdb_create_checkpoint(m_full_dir.c_str());
m_active = (result == HA_EXIT_SUCCESS);
return m_active ? 0 : ER_INTERNAL_ERROR;
}
Expand All @@ -77,21 +79,22 @@ class [[nodiscard]] rdb_checkpoint final {
[[nodiscard]] int cleanup() {
if (!m_active) return 0;
m_active = false;
return myrocks::rocksdb_remove_checkpoint(m_dir.c_str()) == HA_EXIT_SUCCESS
return myrocks::rocksdb_remove_checkpoint(m_full_dir.c_str()) ==
HA_EXIT_SUCCESS
? 0
: ER_INTERNAL_ERROR;
}

[[nodiscard]] constexpr const std::string &get_dir() const noexcept {
assert(m_active);
return m_dir;
return m_full_dir;
}

[[nodiscard]] std::string path(const std::string &file_name) const {
// We might be calling this for inactive checkpoint too, if the donor is in
// the middle of a checkpoint roll. The caller will handle any ENOENTs as
// needed.
return myrocks::rdb_concat_paths(m_dir, file_name);
return myrocks::rdb_concat_paths(m_full_dir, file_name);
}

rdb_checkpoint(const rdb_checkpoint &) = delete;
Expand All @@ -102,10 +105,14 @@ class [[nodiscard]] rdb_checkpoint final {
private:
const std::string m_dir;

std::string m_full_dir;

bool m_active = false;

static std::atomic<std::uint64_t> m_next_id;

std::atomic<std::uint64_t> m_next_sub_id{1};
Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated

[[nodiscard]] static std::string make_dir_name(std::uint64_t id) {
const auto base_str = myrocks::clone::checkpoint_base_dir();
const auto id_str = std::to_string(id);
Expand All @@ -120,6 +127,19 @@ class [[nodiscard]] rdb_checkpoint final {
result += id_str;
return result;
}

[[nodiscard]] static std::string make_dir_full_name(std::string base_dir,
Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated
std::uint64_t id) {
const auto id_str = std::to_string(id);
std::string result;
result.reserve(base_dir.length() + id_str.length() +
1); // +1 for '-', the trailing
// '\0' is accounted by the sizeof.
result = base_dir;
result += '-';
result += id_str;
return result;
}
};

std::atomic<std::uint64_t> rdb_checkpoint::m_next_id{1};
Expand Down
35 changes: 21 additions & 14 deletions storage/rocksdb/ha_rocksdb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,20 +476,6 @@ int rocksdb_create_checkpoint(const char *checkpoint_dir_raw) {
return HA_EXIT_FAILURE;
}

int rocksdb_remove_checkpoint(const char *checkpoint_dir_raw) {
const auto checkpoint_dir = rdb_normalize_dir(checkpoint_dir_raw);
LogPluginErrMsg(INFORMATION_LEVEL, ER_LOG_PRINTF_MSG,
"deleting temporary checkpoint in directory : %s\n",
checkpoint_dir.c_str());
const auto status = rocksdb::DestroyDB(checkpoint_dir, rocksdb::Options());
if (status.ok()) {
return HA_EXIT_SUCCESS;
}
my_error(ER_GET_ERRMSG, MYF(0), status.code(), status.ToString().c_str(),
rocksdb_hton_name);
return HA_EXIT_FAILURE;
}

static int rocksdb_create_checkpoint_validate(
THD *const thd MY_ATTRIBUTE((__unused__)),
struct SYS_VAR *const var MY_ATTRIBUTE((__unused__)),
Expand Down Expand Up @@ -1325,6 +1311,27 @@ static void rocksdb_set_io_write_timeout(
RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
}

int rocksdb_remove_checkpoint(const char *checkpoint_dir_raw) {
const auto checkpoint_dir = rdb_normalize_dir(checkpoint_dir_raw);
LogPluginErrMsg(INFORMATION_LEVEL, ER_LOG_PRINTF_MSG,
"deleting temporary checkpoint in directory : %s\n",
checkpoint_dir.c_str());

std::string trash_dir = std::string(rocksdb_datadir) + "/trash";

@laurynas-biveinis laurynas-biveinis Nov 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO this is important to deduplicate with the other trash dir occurence in rocksdb_init_internal, please extract a helper to get the trash dir.

Suggested change
std::string trash_dir = std::string(rocksdb_datadir) + "/trash";
const auto trash_dir = std::string(rocksdb_datadir) + "/trash";

rocksdb::Options op = rocksdb::Options();
Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated
op.sst_file_manager.reset(NewSstFileManager(

@laurynas-biveinis laurynas-biveinis Nov 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not an expert in SST file manager so open questions:

  1. can we save reuse the existing manager from rocksdb_init_internal as singleton?
  2. if no, is it really correct to pass delete_existing_trash = true to this one? Wouldn't this be stepping on the toes of the other SST file manager instances?

@sunshine-Chun sunshine-Chun Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked with Rocksdb team. The trash_dir has actually been deprecated. It's the old way of managing trash files. Nowadays Rocksdb team manage them by renaming the sst files. I will pass nullptr to trash dir and set the delete_existing_trash to false since we are not using them.

@laurynas-biveinis laurynas-biveinis Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked with Rocksdb team. The trash_dir has actually been deprecated. It's the old way of managing trash files. Nowadays Rocksdb team manage them by renaming the sst files. I will pass nullptr to trash dir and set the delete_existing_trash to false since we are not using them.

I see, thank you. Should we convert the existing rocksdb_init_internal usage as well, in this or a follow-up PR? And can we reuse that same manager object?

@sunshine-Chun sunshine-Chun Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes we can convert the existing rocksdb_init_internal usage in a follow-up PR.

And can we reuse that same manager object?

I am open for suggestion. My original thought is that we only have two places use sst_file_manager. Does it worth to maintain a static variable for the usage of these two places?

@laurynas-biveinis laurynas-biveinis Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am open for suggestion. My original thought is that we only have two places use sst_file_manager. Does it worth to maintain a static variable for the usage of these two places?

I am not sure neither. Clone can run many times, and rotate the checkpoint many times too, so dynamically it could be much more than two places. Also the init options are repeated, and probably should be in sync after the rocksdb_init_internal follow-up. A static variable for this would be closely related to rdb static var, which looks fine to me.

@laurynas-biveinis laurynas-biveinis Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Forgot one more thing, is reusing the same object even safe in the first place?

rocksdb_db_options->env, rocksdb_db_options->info_log, trash_dir,
rocksdb_sst_mgr_rate_bytes_per_sec, true /* delete_existing_trash */));
const auto status = rocksdb::DestroyDB(checkpoint_dir, op);

if (status.ok()) {
return HA_EXIT_SUCCESS;
}
my_error(ER_GET_ERRMSG, MYF(0), status.code(), status.ToString().c_str(),
rocksdb_hton_name);
return HA_EXIT_FAILURE;
}

Comment thread
sunshine-Chun marked this conversation as resolved.
Outdated
#endif // !__APPLE__

enum rocksdb_flush_log_at_trx_commit_type : unsigned int {
Expand Down