Support generics for MultiDimension APIs#3026
Merged
wwbmmm merged 2 commits intoapache:masterfrom Jul 23, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MVariable API to support generic label container types and centralizes common static methods under a new MVariableBase class.
- Introduce
MVariableBasefor shared behavior and rename existing static methods (count_exposed,list_exposed,describe_exposed,dump_exposed) to live on it - Convert
MVariableinto a template class parameterized byKeyTypefor label containers and updateMultiDimensionaccordingly - Update tests to exercise the new generic API and adjust CMake and consumers to call through
MVariableBase
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/bvar_mvariable_unittest.cpp | Switched static calls from MVariable to MVariableBase and added label‐container template tests |
| test/CMakeLists.txt | Added additional Darwin linker flags for new symbols |
| src/bvar/variable.cpp | Redirected dump_exposed call to MVariableBase |
| src/bvar/mvariable.h | Defined new MVariableBase, moved copy-control there, and templated MVariable on KeyType |
| src/bvar/mvariable.cpp | Updated all definitions to match MVariableBase signatures |
| src/bvar/multi_dimension.h & _inl.h | Templated MultiDimension on both T and KeyType, added hashing/equality functors |
| src/butil/strings/string_piece.h | Added C++17 std::string_view constructor and conversion operator |
| src/butil/containers/flat_map_inl.h | Removed commented-out logging |
| src/brpc/builtin/prometheus_metrics_service.cpp | Changed MVariable::dump_exposed calls to MVariableBase::dump_exposed |
Comments suppressed due to low confidence (2)
src/bvar/multi_dimension_inl.h:384
- Typo in function name 'is_valid_lables_value'; consider renaming it to 'is_valid_labels_value' for clarity and consistency.
os << "{\"name\" : \"" << this->name() << "\", \"labels\" : [";
src/bvar/multi_dimension_inl.h:386
- [nitpick] Enhance this error message by adding context and separators, e.g.:
LOG(ERROR) << "Invalid labels count: expected " << this->count_labels()
<< " but got " << labels_value.size();
for (auto& label : this->_labels) {
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number:
Problem Summary:
The current mbvar get/set operation APIs all require the construction of std::liststd::string, which requires the allocation of memory and the copying of strings, resulting in noticeable performance overhead.
What is changed and the side effects?
Changed:
The bvar get/set operation API supports more data structures through generics, thus achieving zero copy. Using Container<const char* / std::string_view / butil::StringPieces> can avoid string copying, where Container can be a container such as std::vector, std::set, std::map, etc. Furthermore, std::array and absl::InlinedVector can be used to avoid dynamic heap memory allocation.
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: