-
Notifications
You must be signed in to change notification settings - Fork 455
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
Add the support of ANALYZE command to inspect the performance of RocksDB #1943
Conversation
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.
This is a bit confusing because it does far less than PROFILE
in SQL. Also it might break the previous config about perflevel, so I wonder whether we have better way for this...
Maybe it's better to first having a RAII guard like TiKV: https://github.com/tikv/tikv/blob/master/src/coprocessor/tracker.rs ( We can be easier than them) ? |
Can you elaborate more on this? I didn't quite get the point. |
Hmm we can just leave it here in this patch. I believe something like a :
Might be better? If other command want to debugging, they can also uses this tool? Also we can considering RAII here. |
84c4794
to
b71c53d
Compare
I see what you meant. So just to be clear, do we still need to do this in this patch? Or we wait until when it's actually needed to do so? |
I agree it's not neccessary in this patch. Personally I think metrics can be widely used, but lets first get it in used... cc @PragmaTwice @git-hulk This command looks like SQL "PROFILE" with rocksdb io/perf stats enabled and output. It can be used in some cases, but for some blocking command I don't know whether it's ok. Would you mind take a look? |
@git-hulk @PragmaTwice Hello guys, if you have time could you help review this pr? Thank you :) |
Sure, sorry for missing this message. I will take a look recently. |
b71c53d
to
cb470c0
Compare
@chrisxu333 Can rename |
ce3220b
to
614672d
Compare
Would you mind fix the lint first? |
@chrisxu333 |
614672d
to
25392d3
Compare
Sounds good I just did. |
|
||
auto s = cmd->Parse(command_args_); |
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.
Should we check the arity before Parse
? Since wrong command_args_
might make our program crash
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.
We need to reuse the code in ExecuteCommand rather than write them again here.
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.
I just pushed a newer version which added the arity checking logic. As for reusing / refactoring the code in ExecuteCommand
, maybe it's better to do it in a separate pr? As the ExecuteCommand
code needs to be dissected into several chunks for reusing purpose. @mapleFU @PragmaTwice
25392d3
to
682777c
Compare
for (int i = 1; i < args.size(); ++i) { | ||
command_args_.push_back(args[i]); | ||
} |
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.
nit: we can directly insert
rather than push back
682777c
to
46957db
Compare
Hi @torwig, could you help review this pr whenever you're able to? Thx! |
Could you help review and approve this pr if possible? Thx! |
@chrisxu333 Thanks for your follow-up. This PR looks good to me, to see if @PragmaTwice and @torwig has any comments, if not, we can move forward to merge. |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
This pr tries to close #783 .
To expose essential perf_context and iostats_context from rocksdb, a new command ANALYZE is introduced.
A typical usage would be as follows: