[Enhancement] Support display datacache metircs by http service. #40220
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| ExecEnv* _exec_env; | ||
| }; | ||
|
|
||
| } // namespace starrocks |
There was a problem hiding this comment.
The code snippet provided is a header file for what appears to be part of an HTTP server component in the StarRocks system, and as such, it does not contain the function implementations which would allow for more in-depth bug identification. Most bugs typically reside in the implementation details rather than declarations.
However, if we were to speculate about potential issues based on the header file and common pitfalls:
- The use of bare pointers like
ExecEnv* exec_env;could lead to memory management issues, such as leaks or double deletion, if proper ownership semantics are not followed in the implementation. - Since
DataCacheActioninherits fromHttpHandler, there may be thread-safety concerns if the handler's methods are called concurrently, but without sufficient context and without seeing the rest of the code (particularly the method implementations), it's hard to say whether this will be a problem.
To provide a more accurate analysis, we would need the corresponding source file (.cpp) that includes the implementations of the handle, _handle, _handle_stat, and _handle_error functions.
If you have specific concerns or known problems you want to discuss, please share more context or a portion of the implementation code, so the analysis can be more precise.
| }); | ||
| } | ||
|
|
||
| } // namespace starrocks |
There was a problem hiding this comment.
The most risky bug in this code is:
Possible capture of this pointer by a lambda which can be invoked after the DataCacheAction object has been destroyed.
You can modify the code like this:
void DataCacheAction::_handle_stat(HttpRequest* req, BlockCache* cache) {
auto block_cache_shared = _exec_env->block_cache_shared(); // Presuming that such a method returns a shared_ptr.
_handle(req, [block_cache_shared](rapidjson::Document& root) {
// Now use block_cache_shared instead of directly using the member variable.
// Implement the rest of the code with the understanding that
// the shared_ptr ensures the cache object remains alive as long as it's needed by the lambda.
});
}This change assumes there is a way to retrieve a shared_ptr to the block cache from the execution environment (as alluded to by _exec_env->block_cache_shared()), preventing the cache and any associated resources from being destroyed while still in use by any lambdas capturing the shared pointer.
GavinMar
force-pushed
the
cache_metrics
branch
2 times, most recently
from
01df2bb to
6762f2b
Compare
| @@ -168,8 +168,8 @@ void BlockCache::record_read_cache(size_t size, int64_t lateny_us) { | |||
| _kv_cache->record_read_cache(size, lateny_us); | |||
| } | |||
|
|
|||
| const DataCacheMetrics BlockCache::cache_metrics() const { | |||
| return _kv_cache->cache_metrics(); | |||
| const DataCacheMetrics BlockCache::cache_metrics(int level) const { | |||
There was a problem hiding this comment.
can we use enum instead?
There was a problem hiding this comment.
can we use enum instead?
It is mainly to be consistent with the dependent library interface, and we can replace it after the library is updated.
| _handle_error(req, strings::Substitute("Not support GET method: '$0'", req->uri())); | ||
| } | ||
| } else { | ||
| _handle_error(req, |
There was a problem hiding this comment.
usually if the method is not allowed, it should be a HTTP 405 error. not a HTTP 200 with response body error: not supported ....
Signed-off-by: Gavin <yangguansuo@starrocks.com>
GavinMar
force-pushed
the
cache_metrics
branch
from
6762f2b to
d4b5a1f
Compare
|
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 9 / 112 (08.04%) file detail
|
|
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
) Signed-off-by: Gavin <yangguansuo@starrocks.com> (cherry picked from commit b054af8) # Conflicts: # be/src/block_cache/cachelib_wrapper.cpp # be/src/block_cache/cachelib_wrapper.h # build.sh # run-be-ut.sh
…rRocks#40220) Signed-off-by: Gavin <yangguansuo@starrocks.com> Signed-off-by: liubotao <316945435@qq.com>
Why I'm doing:
We need some more metrics to evaluate the datacache performance. But now it is difficult because we only expose the datacache memory tracker to trace the memory usage.
What I'm doing:
With this PR, we can get detailed datacache metrics by the following URL:
The response looks like below:
{ "status": "NORMAL", "mem_quota_bytes": 5368709120, "mem_used_bytes": 4237890114, "disk_quota_bytes": 214748364800, "disk_used_bytes": 0, "mem_used_rate": 0.79, "disk_used_rate": 0, "disk_spaces": "/home/disk1/datacache:214748364800", "meta_used_bytes": 3123454, "hit_count": 114438, "miss_count": 1126, "hit_rate": 0.99, "hit_bytes": 29809132182, "miss_bytes": 268505666, "hit_count_last_minute": 114061, "miss_count_last_minute": 1126, "hit_bytes_last_minute": 29713540072, "miss_bytes_last_minute": 268505666, "read_mem_bytes": 29809132182, "read_disk_bytes": 0, "write_bytes": 4237890114, "write_success_count": 16268, "write_fail_count": 20856, "remove_bytes": 0, "remove_success_count": 0, "remove_fail_count": 0, "current_reading_count": 2, "current_writing_count": 0, "current_removing_count": 0 }What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: