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.

Sign up for GitHub

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

[WIP]fix: fix rename cmd #165

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

[WIP]fix: fix rename cmd #165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 58 additions & 6 deletions src/storage/src/redis_lists.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,7 @@ Status Redis::RPushx(const Slice& key, const std::vector<std::string>& values, u
}

Status Redis::ListsRename(const Slice& key, Redis* new_inst, const Slice& newkey) {
auto batch = Batch::CreateBatch(this);
std::string meta_value;
uint32_t statistic = 0;
const std::vector<std::string> keys = {key.ToString(), newkey.ToString()};
Expand All @@ -994,18 +995,44 @@ Status Redis::ListsRename(const Slice& key, Redis* new_inst, const Slice& newkey
// copy a new list with newkey
ParsedListsMetaValue parsed_lists_meta_value(&meta_value);
statistic = parsed_lists_meta_value.Count();
s = new_inst->GetDB()->Put(default_write_options_, handles_[kMetaCF], base_meta_newkey.Encode(), meta_value);

// todo if value is too many, will slow to rename
uint32_t version = parsed_lists_meta_value.Version();
uint64_t index = parsed_lists_meta_value.LeftIndex() + 1;
uint64_t right_index = parsed_lists_meta_value.RightIndex() - 1;
ListsDataKey base_lists_data_key(key, version, index);
std::vector<std::string> list_nodes;
rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[kListsDataCF]);
uint64_t current_index = index;
for(iter->Seek(base_lists_data_key.Encode()); iter->Valid() && current_index <= right_index; iter->Next(), current_index++) {
ParsedBaseDataValue parsed_value(iter->value());
list_nodes.push_back(parsed_value.UserValue().ToString());
}
delete iter;

Comment on lines +998 to +1012
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Potential performance issues when renaming large lists

The current implementation loads all list nodes into memory by storing them in list_nodes, which could cause high memory usage and slow performance when renaming large lists. Consider processing the list data in a streaming fashion or using more efficient methods to handle large datasets.

🧰 Tools
🪛 GitHub Actions: kiwi

[error] File has clang-format style issues. The formatting does not match the project's style guidelines.

// write new data value
current_index = index;
for (const auto& node : list_nodes) {
ListsDataKey new_lists_data_key(newkey, version, current_index++);
BaseDataValue n_val(node);
batch->Put(kListsDataCF, new_lists_data_key.Encode(), n_val.Encode());
}
// write new meta_key
batch->Put(kMetaCF, base_meta_newkey.Encode(), meta_value);
new_inst->UpdateSpecificKeyStatistics(DataType::kLists, newkey.ToString(), statistic);

// ListsDel key
parsed_lists_meta_value.InitialMetaValue();
s = db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value);
batch->Delete(kListsDataCF, base_meta_key.Encode());
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure atomicity by including old key metadata update in the batch

The update to the old key's metadata on line 1027 is performed outside of the batch using db_->Put. This could result in an inconsistent state if an error occurs after this operation but before the batch commit. To ensure atomicity of the rename operation, include this update within the batch.

Apply this diff to include the metadata update in the batch:

-  s = db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value);
+  batch->Put(kMetaCF, base_meta_key.Encode(), meta_value);

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Actions: kiwi

[error] File has clang-format style issues. The formatting does not match the project's style guidelines.

UpdateSpecificKeyStatistics(DataType::kLists, key.ToString(), statistic);

return s;
return batch->Commit();
}

Status Redis::ListsRenamenx(const Slice& key, Redis* new_inst, const Slice& newkey) {
auto batch = Batch::CreateBatch(this);

std::string meta_value;
uint32_t statistic = 0;
const std::vector<std::string> keys = {key.ToString(), newkey.ToString()};
Expand All @@ -1029,22 +1056,47 @@ Status Redis::ListsRenamenx(const Slice& key, Redis* new_inst, const Slice& newk
ParsedListsMetaValue parsed_lists_meta_value(&meta_value);
s = new_inst->GetDB()->Get(default_read_options_, handles_[kMetaCF], base_meta_newkey.Encode(), &new_meta_value);
if (s.ok()) {
if (IsStale(new_meta_value)) {
ParsedListsMetaValue parsed_lists_new_meta_value(new_meta_value);
if (parsed_lists_new_meta_value.Count() != 0 || parsed_lists_new_meta_value.IsStale()) {
return Status::Corruption(); // newkey already exists.
}
}
ParsedSetsMetaValue parsed_lists_new_meta_value(&new_meta_value);
// ParsedListsMetaValue parsed_lists_new_meta_value(&new_meta_value);
// copy a new list with newkey
statistic = parsed_lists_meta_value.Count();
s = new_inst->GetDB()->Put(default_write_options_, handles_[kMetaCF], base_meta_newkey.Encode(), meta_value);

// todo if value is too many, will slow to rename
uint32_t version = parsed_lists_meta_value.Version();
uint64_t index = parsed_lists_meta_value.LeftIndex() + 1;
uint64_t right_index = parsed_lists_meta_value.RightIndex() - 1;
ListsDataKey base_lists_data_key(key, version, index);
std::vector<std::string> list_nodes;
rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[kListsDataCF]);
uint64_t current_index = index;
for(iter->Seek(base_lists_data_key.Encode()); iter->Valid() && current_index <= right_index; iter->Next(), current_index++) {
ParsedBaseDataValue parsed_value(iter->value());
list_nodes.push_back(parsed_value.UserValue().ToString());
}
delete iter;

// write new data value
current_index = index;
for (const auto& node : list_nodes) {
ListsDataKey new_lists_data_key(newkey, version, current_index++);
BaseDataValue n_val(node);
batch->Put(kListsDataCF, new_lists_data_key.Encode(), n_val.Encode());
}
// write new meta_key
batch->Put(kMetaCF, base_meta_newkey.Encode(), meta_value);
new_inst->UpdateSpecificKeyStatistics(DataType::kLists, newkey.ToString(), statistic);

// ListsDel key
parsed_lists_meta_value.InitialMetaValue();
s = db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value);
batch->Delete(kListsDataCF, base_meta_key.Encode());
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure atomicity by including old key metadata update in the batch

Similarly, in the ListsRenamenx method, the update to the old key's metadata is performed outside of the batch using db_->Put. This could lead to inconsistent states if an error occurs. Include this operation in the batch to maintain atomicity.

Apply this diff to include the metadata update in the batch:

-  s = db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value);
+  batch->Put(kMetaCF, base_meta_key.Encode(), meta_value);

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Actions: kiwi

[error] File has clang-format style issues. The formatting does not match the project's style guidelines.

UpdateSpecificKeyStatistics(DataType::kLists, key.ToString(), statistic);

return s;
return batch->Commit();
}

void Redis::ScanLists() {
Expand Down
Loading