-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: supported 'ptype' command #1586
Changes from all commits
c67ac2f
9ec9485
99a97b2
ed7cca2
e691134
e0bdee6
a16858a
424fb93
9db60db
2b7032f
3f577ff
b7ecdd0
34c5de2
336c931
376c70f
ef016fb
8f86adc
d3a01d8
87c0432
4f5314e
23ff105
0888591
7bf5afb
016d6fe
b14c3fd
d110dc6
e480f24
6072bc0
2f3db19
da9b876
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
--- | ||
name: Bug report | ||
about: Create a report to help us improve | ||
title: '' | ||
labels: '' | ||
assignees: '' | ||
|
||
--- | ||
|
||
**Describe the bug** | ||
A clear and concise description of what the bug is. | ||
|
||
**To Reproduce** | ||
Steps to reproduce the behavior: | ||
1. Go to '...' | ||
2. Click on '....' | ||
3. Scroll down to '....' | ||
4. See error | ||
|
||
**Expected behavior** | ||
A clear and concise description of what you expected to happen. | ||
|
||
**Screenshots** | ||
If applicable, add screenshots to help explain your problem. | ||
|
||
**Desktop (please complete the following information):** | ||
- OS: [e.g. iOS] | ||
- Browser [e.g. chrome, safari] | ||
- Version [e.g. 22] | ||
|
||
**Smartphone (please complete the following information):** | ||
- Device: [e.g. iPhone6] | ||
- OS: [e.g. iOS8.1] | ||
- Browser [e.g. stock browser, safari] | ||
- Version [e.g. 22] | ||
|
||
**Additional context** | ||
Add any other context about the problem here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
--- | ||
name: Feature request | ||
about: Suggest an idea for this project | ||
title: '' | ||
labels: '' | ||
assignees: '' | ||
|
||
--- | ||
|
||
**Is your feature request related to a problem? Please describe.** | ||
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] | ||
|
||
**Describe the solution you'd like** | ||
A clear and concise description of what you want to happen. | ||
|
||
**Describe alternatives you've considered** | ||
A clear and concise description of any alternative solutions or features you've considered. | ||
|
||
**Additional context** | ||
Add any other context or screenshots about the feature request here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,7 @@ const std::string kCmdNameTtl = "ttl"; | |
const std::string kCmdNamePttl = "pttl"; | ||
const std::string kCmdNamePersist = "persist"; | ||
const std::string kCmdNameType = "type"; | ||
const std::string kCmdNamePType = "ptype"; | ||
const std::string kCmdNameScan = "scan"; | ||
const std::string kCmdNameScanx = "scanx"; | ||
const std::string kCmdNamePKSetexAt = "pksetexat"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the provided code patch, here are some observations:
Improvement suggestion:
Bug risk assessment:
Overall, this code patch seems relatively safe and straightforward, with only a minor improvement suggestion regarding naming conventions. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -588,6 +588,24 @@ class TypeCmd : public Cmd { | |
void DoInitial() override; | ||
}; | ||
|
||
class PTypeCmd : public Cmd { | ||
public: | ||
PTypeCmd(const std::string& name, int arity, uint16_t flag) : Cmd(name, arity, flag) {} | ||
std::vector<std::string> current_key() const override { | ||
std::vector<std::string> res; | ||
res.push_back(key_); | ||
return res; | ||
} | ||
void Do(std::shared_ptr<Slot> slot = nullptr) override; | ||
void Split(std::shared_ptr<Slot> slot, const HintKeys& hint_keys) override {}; | ||
void Merge() override {}; | ||
Cmd* Clone() override { return new PTypeCmd(*this); } | ||
|
||
private: | ||
std::string key_; | ||
void DoInitial() override; | ||
}; | ||
|
||
class ScanCmd : public Cmd { | ||
public: | ||
ScanCmd(const std::string& name, int arity, uint16_t flag) : Cmd(name, arity, flag), pattern_("*") {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code patch seems to add a new class However, it would be helpful to have some more context on the purpose of this new class and how it fits into the existing codebase. Additionally, it's worth noting that brief code reviews are often insufficient for thorough evaluation of code quality, maintainability, and scalability. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -280,6 +280,9 @@ void InitCmdTable(CmdTable* cmd_table) { | |
std::unique_ptr<Cmd> typeptr = | ||
std::make_unique<TypeCmd>(kCmdNameType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv); | ||
cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameType, std::move(typeptr))); | ||
////PTypeCmd | ||
std::unique_ptr<Cmd> pTypeptr = std::make_unique<PTypeCmd>(kCmdNamePType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv); | ||
cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNamePType, std::move(pTypeptr))); | ||
////ScanCmd | ||
std::unique_ptr<Cmd> scanptr = | ||
std::make_unique<ScanCmd>(kCmdNameScan, -2, kCmdFlagsRead | kCmdFlagsMultiSlot | kCmdFlagsKv); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the provided code patch, here is a brief code review:
Some general suggestions for improvement:
Remember to thoroughly test the modified code to ensure it behaves as expected and doesn't introduce any bugs or issues. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,9 +184,7 @@ void DelCmd::Split(std::shared_ptr<Slot> slot, const HintKeys& hint_keys) { | |
} | ||
} | ||
|
||
void DelCmd::Merge() { | ||
res_.AppendInteger(split_res_); | ||
} | ||
void DelCmd::Merge() { res_.AppendInteger(split_res_); } | ||
|
||
void IncrCmd::DoInitial() { | ||
if (!CheckArg(argv_.size())) { | ||
|
@@ -804,9 +802,7 @@ void ExistsCmd::Split(std::shared_ptr<Slot> slot, const HintKeys& hint_keys) { | |
} | ||
} | ||
|
||
void ExistsCmd::Merge() { | ||
res_.AppendInteger(split_res_); | ||
} | ||
void ExistsCmd::Merge() { res_.AppendInteger(split_res_); } | ||
|
||
void ExpireCmd::DoInitial() { | ||
if (!CheckArg(argv_.size())) { | ||
|
@@ -1088,10 +1084,33 @@ void TypeCmd::DoInitial() { | |
} | ||
|
||
void TypeCmd::Do(std::shared_ptr<Slot> slot) { | ||
std::string res; | ||
rocksdb::Status s = slot->db()->Type(key_, &res); | ||
std::vector<std::string> types(1); | ||
rocksdb::Status s = slot->db()->GetType(key_, true, types); | ||
if (s.ok()) { | ||
res_.AppendContent("+" + res); | ||
res_.AppendContent("+" + types[0]); | ||
} else { | ||
res_.SetRes(CmdRes::kErrOther, s.ToString()); | ||
} | ||
} | ||
|
||
void PTypeCmd::DoInitial() { | ||
if (!CheckArg(argv_.size())) { | ||
res_.SetRes(CmdRes::kWrongNum, kCmdNameType); | ||
return; | ||
} | ||
key_ = argv_[1]; | ||
} | ||
|
||
void PTypeCmd::Do(std::shared_ptr<Slot> slot) { | ||
std::vector<std::string> types(5); | ||
rocksdb::Status s = slot->db()->GetType(key_, false, types); | ||
|
||
if (s.ok()) { | ||
res_.AppendArrayLen(types.size()); | ||
for (const auto& vs : types) { | ||
res_.AppendStringLen(vs.size()); | ||
res_.AppendContent(vs); | ||
} | ||
} else { | ||
res_.SetRes(CmdRes::kErrOther, s.ToString()); | ||
} | ||
|
@@ -1328,8 +1347,7 @@ void PKScanRangeCmd::Do(std::shared_ptr<Slot> slot) { | |
std::string next_key; | ||
std::vector<std::string> keys; | ||
std::vector<storage::KeyValue> kvs; | ||
rocksdb::Status s = | ||
slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key); | ||
rocksdb::Status s = slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key); | ||
|
||
if (s.ok()) { | ||
res_.AppendArrayLen(2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, the code patch seems to involve mostly formatting changes and a few small modifications. Here are some potential bug risks and improvement suggestions for the code:
Please note that this review is based solely on the provided code patch, and other parts of the codebase might also require review and consideration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the code patch you provided, here are some observations and suggestions:
It's challenging to conduct a comprehensive code review based on a small code patch. It's crucial to consider the context, purpose, and requirements of the code before making any changes. Additionally, testing and validating the modified code is essential to ensure it functions correctly and doesn't introduce regressions. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -662,9 +662,9 @@ int PikaMigrateThread::ReqMigrateOne(const std::string &key, const std::shared_p | |
std::unique_lock lm(migrator_mutex_); | ||
|
||
int slot_id = GetSlotID(key); | ||
std::string type_str; | ||
std::vector<std::string> type_str(1); | ||
char key_type; | ||
rocksdb::Status s = slot->db()->Type(key, &type_str); | ||
rocksdb::Status s = slot->db()->GetType(key, true, type_str); | ||
if (!s.ok()) { | ||
if (s.IsNotFound()) { | ||
LOG(INFO) << "PikaMigrateThread::ReqMigrateOne key: " << key << " not found"; | ||
|
@@ -675,20 +675,20 @@ int PikaMigrateThread::ReqMigrateOne(const std::string &key, const std::shared_p | |
} | ||
} | ||
|
||
if (type_str == "string") { | ||
if (type_str[0] == "string") { | ||
key_type = 'k'; | ||
} else if (type_str == "hash") { | ||
} else if (type_str[0] == "hash") { | ||
key_type = 'h'; | ||
} else if (type_str == "list") { | ||
} else if (type_str[0] == "list") { | ||
key_type = 'l'; | ||
} else if (type_str == "set") { | ||
} else if (type_str[0] == "set") { | ||
key_type = 's'; | ||
} else if (type_str == "zset") { | ||
} else if (type_str[0] == "zset") { | ||
key_type = 'z'; | ||
} else if (type_str == "none") { | ||
} else if (type_str[0] == "none") { | ||
return 0; | ||
} else { | ||
LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: " << key << " type: " << type_str << " is illegal"; | ||
LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: " << key << " type: " << type_str[0] << " is illegal"; | ||
return -1; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are some observations and suggestions regarding the provided code patch:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -891,22 +891,22 @@ void RemSlotKey(const std::string key, const std::shared_ptr<Slot>& slot) { | |
} | ||
|
||
int GetKeyType(const std::string key, std::string &key_type, const std::shared_ptr<Slot>& slot) { | ||
std::string type_str; | ||
rocksdb::Status s = slot->db()->Type(key, &type_str); | ||
std::vector<std::string> type_str(1); | ||
rocksdb::Status s = slot->db()->GetType(key, true, type_str); | ||
if (!s.ok()) { | ||
LOG(WARNING) << "Get key type error: " << key << " " << s.ToString(); | ||
key_type = ""; | ||
return -1; | ||
} | ||
if (type_str == "string") { | ||
if (type_str[0] == "string") { | ||
key_type = "k"; | ||
} else if (type_str == "hash") { | ||
} else if (type_str[0] == "hash") { | ||
key_type = "h"; | ||
} else if (type_str == "list") { | ||
} else if (type_str[0] == "list") { | ||
key_type = "l"; | ||
} else if (type_str == "set") { | ||
} else if (type_str[0] == "set") { | ||
key_type = "s"; | ||
} else if (type_str == "zset") { | ||
} else if (type_str[0] == "zset") { | ||
key_type = "z"; | ||
} else { | ||
LOG(WARNING) << "Get key type error: " << key; | ||
|
@@ -1079,8 +1079,8 @@ void SlotsMgrtTagSlotCmd::Do(std::shared_ptr<Slot> slot) { | |
|
||
// check key type | ||
int SlotsMgrtTagOneCmd::KeyTypeCheck(const std::shared_ptr<Slot>& slot) { | ||
std::string type_str; | ||
rocksdb::Status s = slot->db()->Type(key_, &type_str); | ||
std::vector<std::string> type_str(1); | ||
rocksdb::Status s = slot->db()->GetType(key_, true, type_str); | ||
if (!s.ok()) { | ||
if (s.IsNotFound()) { | ||
LOG(INFO) << "Migrate slot key " << key_ << " not found"; | ||
|
@@ -1091,15 +1091,15 @@ int SlotsMgrtTagOneCmd::KeyTypeCheck(const std::shared_ptr<Slot>& slot) { | |
} | ||
return -1; | ||
} | ||
if (type_str == "string") { | ||
if (type_str[0] == "string") { | ||
key_type_ = 'k'; | ||
} else if (type_str == "hash") { | ||
} else if (type_str[0] == "hash") { | ||
key_type_ = 'h'; | ||
} else if (type_str == "list") { | ||
} else if (type_str[0] == "list") { | ||
key_type_ = 'l'; | ||
} else if (type_str == "set") { | ||
} else if (type_str[0] == "set") { | ||
key_type_ = 's'; | ||
} else if (type_str == "zset") { | ||
} else if (type_str[0] == "zset") { | ||
key_type_ = 'z'; | ||
} else { | ||
LOG(WARNING) << "Migrate slot key: " << key_ << " not found"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the code patch you provided, here are some observations and suggestions:
Overall, from the code patch you provided, it appears that the changes made relate to modifying the way key types are stored and retrieved. The actual correctness of these changes depends on the implementation details of the |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,8 +131,7 @@ struct BGTask { | |
Operation operation; | ||
std::string argv; | ||
|
||
BGTask(const DataType& _type = DataType::kAll, const Operation& _opeation = Operation::kNone, | ||
std::string _argv = "") | ||
BGTask(const DataType& _type = DataType::kAll, const Operation& _opeation = Operation::kNone, std::string _argv = "") | ||
: type(_type), operation(_opeation), argv(std::move(_argv)) {} | ||
}; | ||
|
||
|
@@ -424,7 +423,7 @@ class Storage { | |
|
||
// Removes and returns several random elements specified by count from the set value store at key. | ||
Status SPop(const Slice& key, std::vector<std::string>* members, int64_t count); | ||
|
||
// When called with just the key argument, return a random element from the | ||
// set value stored at key. | ||
// when called with the additional count argument, return an array of count | ||
|
@@ -968,8 +967,12 @@ class Storage { | |
// return > 0 TTL in seconds | ||
std::map<DataType, int64_t> TTL(const Slice& key, std::map<DataType, Status>* type_status); | ||
|
||
// Reutrns the data type of the key | ||
Status Type(const std::string& key, std::string* type); | ||
// Reutrns the data all type of the key | ||
// if single is true, the query will return the first one | ||
Status GetType(const std::string& key, bool single, std::vector<std::string>& types); | ||
|
||
// Reutrns the data all type of the key | ||
Status Type(const std::string& key, std::vector<std::string>& types); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 函数名可能需要更具体点,与上面Type()区分。 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 嗯, 和Ptype保持一致会更好点 |
||
|
||
Status Keys(const DataType& data_type, const std::string& pattern, std::vector<std::string>* keys); | ||
|
||
|
@@ -1019,29 +1022,29 @@ class Storage { | |
|
||
Status SetOptions(const OptionType& option_type, const std::string& db_type, | ||
const std::unordered_map<std::string, std::string>& options); | ||
void GetRocksDBInfo(std::string &info); | ||
void GetRocksDBInfo(std::string& info); | ||
|
||
private: | ||
std::unique_ptr<RedisStrings> strings_db_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, the code changes appear to be minor and mostly related to formatting. However, there are a couple of observations:
Apart from these observations, no significant bug risks or improvement suggestions stand out from the code patch. |
||
std::unique_ptr<RedisHashes> hashes_db_; | ||
std::unique_ptr<RedisSets> sets_db_; | ||
std::unique_ptr<RedisZSets> zsets_db_; | ||
std::unique_ptr<RedisLists> lists_db_; | ||
std::atomic<bool> is_opened_; | ||
std::atomic<bool> is_opened_ = false; | ||
|
||
std::unique_ptr<LRUCache<std::string, std::string>> cursors_store_; | ||
|
||
// Storage start the background thread for compaction task | ||
pthread_t bg_tasks_thread_id_; | ||
pthread_t bg_tasks_thread_id_ = 0; | ||
pstd::Mutex bg_tasks_mutex_; | ||
pstd::CondVar bg_tasks_cond_var_; | ||
std::queue<BGTask> bg_tasks_queue_; | ||
|
||
std::atomic<int> current_task_type_; | ||
std::atomic<bool> bg_tasks_should_exit_; | ||
std::atomic<int> current_task_type_ = kNone; | ||
std::atomic<bool> bg_tasks_should_exit_ = false; | ||
|
||
// For scan keys in data base | ||
std::atomic<bool> scan_keynum_exit_; | ||
std::atomic<bool> scan_keynum_exit_ = false; | ||
}; | ||
|
||
} // namespace storage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are my observations and suggestions:
Overall, the code seems reasonable, and I don't see any obvious bug risks. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's difficult to fully review the code patch without the context of the entire source code. However, based on this snippet:
kCmdNamePType
has been added.kCmdNameTtl
,kCmdNamePttl
,kCmdNamePersist
,kCmdNameType
,kCmdNameScan
,kCmdNameScanx
, andkCmdNamePKSetexAt
are still present.Here are some general suggestions for code review: