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

Jump to bottom

Feat fixed unit basic #1640

Merged
merged 4 commits into from
Jun 25, 2023
Merged

Feat fixed unit basic #1640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49b7341
fixed: dbsize ret 0
wang1309 Jun 23, 2023
da5e241
fixed: modify INCRBYFLOAT error prompt info
wang1309 Jun 23, 2023
9c6e423
fixed: comment debug unit test case and modify incrbyfloat overflow t…
wang1309 Jun 24, 2023
4901cfa
fixed: dbsize get zero fix
wang1309 Jun 24, 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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,4 @@ deps
# include codis fe javascript lib files
!codis/cmd/fe/assets/**

tests/tmp
Copy link

Choose a reason for hiding this comment

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

The provided code patch only adds a new line to the file, which includes the path "tests/tmp". It is difficult to provide any insights regarding potential bugs or improvements without more context about the purpose and functionality of this file.

1 change: 1 addition & 0 deletions src/pika_admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,7 @@ void DbsizeCmd::Do(std::shared_ptr<Slot> slot) {
if (!db) {
res_.SetRes(CmdRes::kInvalidDB);
} else {
db->RunKeyScan();
Copy link
Collaborator

Choose a reason for hiding this comment

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

pika 设计并不支持实时统计，因为扫描有锁且耗时，最好还是在后台起线程做。可以参考 docs/ops/bestPractice.md pika最佳实践之十六.

针对测试修复，可以在 tcl 脚本的 dbsize 前添加 r info keyspace 1 来触发 keyscan。
另外由于 keyscan 为异步操作，可能触发后并不会立即完成，可以在r dbsize 前添加 after 1000 来 +1s。
更优雅的做法是实现一个函数来等待扫描结束，函数内一直循环直到 is_scaning_keyspace 为 No，可以参考 tests/support/util.tcl 中的 proc waitForBgsave，不过测试数据量不大，after即可 。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update

KeyScanInfo key_scan_info = db->GetKeyScanInfo();
std::vector<storage::KeyInfo> key_infos = key_scan_info.key_infos;
if (key_infos.size() != 5) {
Copy link

Choose a reason for hiding this comment

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

The code patch seems to add a call to db->RunKeyScan() before retrieving the key scan information using db->GetKeyScanInfo(). It's hard to tell if this is appropriate without seeing the implementation of these methods, but assuming they are implemented correctly with proper error handling, this change should not introduce any bugs.

As for improvement suggestions, it would be good to replace the hard-coded value 5 with a constant or variable that has a meaningful name and is defined elsewhere in the code, so that it is easier to change in the future if necessary. Additionally, if key_infos is only being used to check its size, you could use key_scan_info.key_count instead of creating a vector just to get the size. Finally, it might be worth considering adding some comments to explain the purpose of db->RunKeyScan(), since it is not obvious from the name what it does.

Expand Down
2 changes: 1 addition & 1 deletion tests/unit/basic.tcl
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

非差异化命令的错误信息应与 redis 对齐，因为客户端可能会依赖错误信息进行进一步的处理。
可以修改 pika 的错误信息，而不是单测。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ start_server {tags {"basic"}} {
# p.s. no way I can force NaN to test it from the API because
# there is no way to increment / decrement by infinity nor to
# perform divisions.
} {ERR*would produce*}
} {ERR*increment or decrement would overflow*}

test {INCRBYFLOAT decrement} {
r set foo 1
Copy link

Choose a reason for hiding this comment

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

The code patch only includes one modification, changing an error message string from "ERRwould produce" to "ERRincrement or decrement would overflow". This change is reasonable and seems to be well-focused on a specific issue.

However, it's hard to tell whether there are other bugs or improvement suggestions without more context about the surrounding code.

Expand Down