Simplify TraceAnalyzer
#8697
Simplify TraceAnalyzer
#8697
Conversation
| if (!ta_[TraceOperationType::kGet].enabled) { | ||
| return Status::OK(); | ||
| } | ||
| value_size = 10; |
There was a problem hiding this comment.
@zhichao-cao Why change value size to 10?
There was a problem hiding this comment.
In the Get trace record, we do not collect the value. So, we do not have the "value" part. In order to simplify the later on calculation, we just give a fixed value. Maybe it would be better to add a comments here to make it more clear
There was a problem hiding this comment.
One more question, why we don't do the same thing for other key only operations (seeks, deletes, etc.)?
There was a problem hiding this comment.
obviously, we forgot to do the same thing for other non-value recorded operations........
tools/trace_analyzer_tool.h
Outdated
| class TraceAnalysisResult : public TraceRecordResult { | ||
| public: | ||
| // All operations must have the same timestamp and trace type, e.g, MultiGet. | ||
| TraceAnalysisResult(TraceOperationType op_type, std::vector<uint32_t> cf_ids, |
There was a problem hiding this comment.
What if in the future, we wan to get some special results from different types? E.g., for iterator, we my need to get the lower and upper bound? So, is a general TraceAnalysisResult enough?
There was a problem hiding this comment.
A general TraceAnalysisResult may not be enough for all potential special requirements, but at least now it's better to have individual functions with mostly the same code for each operation type.
As for iterator's lower/upper bound, they are used when creating an iterator. I guess later we may need to have some iterator creation trace type, then, the 2 bounds can be handled there.
tools/trace_analyzer_tool.cc
Outdated
| return write_result_->Accept(this); | ||
| } | ||
|
|
||
| Status TraceAnalyzer::Handle(const TraceAnalysisResult& result) { |
There was a problem hiding this comment.
Need some comments here to explain what is the job of this "Handle".
There was a problem hiding this comment.
ACK.
It is used to handle the generic analysis result for outputting purpose. If we don't use TraceAnalysisResult, we can probably replace it with a single function that does the same job (actually this should be better).
autopear
changed the title
TraceAnalyzer to use TraceRecordResultTraceAnalyzer
| @@ -169,6 +168,8 @@ DEFINE_double(sample_ratio, 1.0, | |||
|
|
|||
| namespace ROCKSDB_NAMESPACE { | |||
|
|
|||
| const size_t kShadowValueSize = 10; | |||
There was a problem hiding this comment.
Maybe add comment to explain why we need it.
There was a problem hiding this comment.
There's a brief description in the header file.
|
@autopear has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@autopear has updated the pull request. You must reimport the pull request before landing. |
|
@autopear has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Handler functions now use a common output function to output to stdout/files. Pull Request resolved: facebook/rocksdb#8697 Test Plan: `trace_analyzer_test` can pass. Reviewed By: zhichao-cao Differential Revision: D30527696 Pulled By: autopear fbshipit-source-id: c626cf4d53a39665a9c4bcf0cb019c448434abe4
Handler functions now use a common output function to output to stdout/files.
Test:
trace_analyzer_testcan pass.