You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pegasus.apache.org by wa...@apache.org on 2023/05/31 14:06:20 UTC
[incubator-pegasus] branch migrate-metrics-dev updated: refactor(new_metrics): refactor enum definition for metric types and units (#1491)
This is an automated email from the ASF dual-hosted git repository.
wangdan pushed a commit to branch migrate-metrics-dev
in repository https://gitbox.apache.org/repos/asf/incubator-pegasus.git
The following commit(s) were added to refs/heads/migrate-metrics-dev by this push:
new 0d616a4b3 refactor(new_metrics): refactor enum definition for metric types and units (#1491)
0d616a4b3 is described below
commit 0d616a4b3c5305faba1e043a7e7bf5a96004bc83
Author: Dan Wang <wa...@apache.org>
AuthorDate: Wed May 31 22:06:13 2023 +0800
refactor(new_metrics): refactor enum definition for metric types and units (#1491)
https://github.com/apache/incubator-pegasus/issues/1490
For the enum classes, such as metric type and unit, in addition to the declaration
of all enumerators, extra code is needed to implement `enum_to_string` for each
enumerator. However, the "extra code" tended to be forgotten, just as what has
been said in the above issue #1490.
Therefore, we should find a way that could share all the enumerators between
the declaration and `enum_to_string`. Each enumerator would be written only
once, and there is no need to remember to add `enum_to_string` for each
enumerator.
We could implement the "sharing" by function-like macros. They have the names
of all the enumerators, and accept an argument to perform custom actions over
the names to implement the "sharing". Both metric types and units would be
refactored in this way.
While building ASAN Github actions failed due to running out of disk space.
This problem is resolved by dropping all directories of `CMakeFiles`, which is
also tracked another issue:
https://github.com/apache/incubator-pegasus/issues/1497
---
.github/workflows/lint_and_test_cpp.yaml | 9 +-
src/replica/test/replica_disk_test_base.h | 1 -
src/runtime/task/task_queue.cpp | 2 +-
src/utils/enum_helper.h | 63 ++++++++-
src/utils/metrics.h | 107 +++++++--------
src/utils/test/enum_helper_test.cpp | 212 ++++++++++++++++++++++++++++++
6 files changed, 334 insertions(+), 60 deletions(-)
diff --git a/.github/workflows/lint_and_test_cpp.yaml b/.github/workflows/lint_and_test_cpp.yaml
index 49a136a84..d6680f175 100644
--- a/.github/workflows/lint_and_test_cpp.yaml
+++ b/.github/workflows/lint_and_test_cpp.yaml
@@ -159,7 +159,8 @@ jobs:
mv thirdparty/hadoop-bin ./
mv thirdparty/zookeeper-bin ./
rm -rf thirdparty
- tar --exclude='*CMakeFiles*' -zcvhf release__builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
+ find ./build/latest/src/ -name '*CMakeFiles*' -type d -exec rm -rf "{}" +
+ tar -zcvhf release__builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
- name: Upload Artifact
uses: actions/upload-artifact@v3
with:
@@ -288,7 +289,8 @@ jobs:
mv thirdparty/hadoop-bin ./
mv thirdparty/zookeeper-bin ./
rm -rf thirdparty
- tar --exclude='*CMakeFiles*' -zcvhf release_address_builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
+ find ./build/latest/src/ -name '*CMakeFiles*' -type d -exec rm -rf "{}" +
+ tar -zcvhf release_address_builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
- name: Upload Artifact
uses: actions/upload-artifact@v3
with:
@@ -557,7 +559,8 @@ jobs:
mv thirdparty/hadoop-bin ./
mv thirdparty/zookeeper-bin ./
rm -rf thirdparty
- tar --exclude='*CMakeFiles*' -zcvhf release_jemalloc_builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
+ find ./build/latest/src/ -name '*CMakeFiles*' -type d -exec rm -rf "{}" +
+ tar -zcvhf release_jemalloc_builder.tar build/latest/output build/latest/bin build/latest/src/server/test/config.ini hadoop-bin zookeeper-bin
- name: Upload Artifact
uses: actions/upload-artifact@v3
with:
diff --git a/src/replica/test/replica_disk_test_base.h b/src/replica/test/replica_disk_test_base.h
index 2da9d9276..1cc8a4fa9 100644
--- a/src/replica/test/replica_disk_test_base.h
+++ b/src/replica/test/replica_disk_test_base.h
@@ -57,7 +57,6 @@ public:
generate_mock_app_info();
stub->_fs_manager._dir_nodes.clear();
- stub->_fs_manager.reset_disk_stat();
generate_mock_dir_nodes(dir_nodes_count);
generate_mock_empty_dir_node(empty_dir_nodes_count);
diff --git a/src/runtime/task/task_queue.cpp b/src/runtime/task/task_queue.cpp
index 19de1954c..425880909 100644
--- a/src/runtime/task/task_queue.cpp
+++ b/src/runtime/task/task_queue.cpp
@@ -55,7 +55,7 @@ METRIC_DEFINE_counter(queue,
METRIC_DEFINE_counter(queue,
queue_rejected_tasks,
dsn::metric_unit::kTasks,
- "The accumulative number of rejeced tasks by throttling before enqueue");
+ "The accumulative number of rejected tasks by throttling before enqueue");
namespace dsn {
diff --git a/src/utils/enum_helper.h b/src/utils/enum_helper.h
index 25d78b909..fd9ba764f 100644
--- a/src/utils/enum_helper.h
+++ b/src/utils/enum_helper.h
@@ -36,9 +36,14 @@
#pragma once
#include <map>
-#include <string>
-#include <mutex>
#include <memory>
+#include <mutex>
+#include <string>
+
+namespace dsn {
+template <typename TEnum>
+class enum_helper_xxx;
+} // namespace dsn
// an invalid enum value must be provided so as to be the default value when parsing failed
#define ENUM_BEGIN2(type, name, invalid_value) \
@@ -52,6 +57,8 @@
#define ENUM_REG_WITH_CUSTOM_NAME(type, name) helper->register_enum(#name, type);
#define ENUM_REG(e) helper->register_enum(#e, e);
+// Argument `type invalid_value` for enum_from_string, albeit unused, has to be provided due to
+// overloading.
#define ENUM_END2(type, name) \
return helper; \
} \
@@ -66,6 +73,58 @@
#define ENUM_END(type) ENUM_END2(type, type)
+// Google Style (https://google.github.io/styleguide/cppguide.html#Enumerator_Names) has recommended
+// using k-prefixed camelCase to name an enumerator instead of MACRO_CASE. That is, use kEnumName,
+// rather than ENUM_NAME.
+//
+// On the other hand, the string representation for an enumerator is often needed. After declaring
+// the enumerator, ENUM_REG* macros would also be used to register the string representation, which
+// has some drawbacks:
+// * the enumerator has to appear again, leading to redundant code;
+// * once there are numerous enumerators, ENUM_REG* tend to be forgotten and the registers for the
+// string representation would be missing.
+//
+// To solve these problems, ENUM_CONST* macros are introduced:
+// * firstly, naming for the string representation should be UpperCamelCase style (namely EnumName);
+// * ENUM_CONST() could be used to generate the enumerators according to the string representation;
+// * only support `enum class` declarations;
+// * ENUM_CONST_DEF() could be used to declare enumerators in the enum class;
+// * ENUM_CONST_REG_STR could be used to register the string representation.
+//
+// The usage of these macros would be described as below. For example, Status::code of rocksdb
+// (include/rocksdb/status.h) could be defined by ENUM_CONST* as following steps:
+//
+// 1. List string representation for each enumerator by a user-defined macro:
+// --------------------------------------------------------------------------
+/*
+ * #define ENUM_FOREACH_STATUS_CODE(DEF) \
+ * DEF(Ok) \
+ * DEF(NotFound) \
+ * DEF(Corruption) \
+ * DEF(IOError)
+ */
+// 2. Declare an enum class by above user-defined macro, with an Invalid and an Count enumerator if
+// necessary:
+// ------------------------------------------------------------------------------------------------
+// enum class status_code
+// {
+// ENUM_FOREACH_STATUS_CODE(ENUM_CONST_DEF) kCount, kInvalidCode,
+// };
+//
+// 3. Define another user-defined macro to register string representations:
+// ------------------------------------------------------------------------
+// #define ENUM_CONST_REG_STR_STATUS_CODE(str) ENUM_CONST_REG_STR(status_code, str)
+//
+// 4. Define enum helper class:
+// ----------------------------
+// ENUM_BEGIN(status_code, status_code::kInvalidCode)
+// ENUM_FOREACH_STATUS_CODE(ENUM_CONST_REG_STR_STATUS_CODE)
+// ENUM_END(status_code)
+#define ENUM_CONST(str) k##str
+#define ENUM_CONST_DEF(str) ENUM_CONST(str),
+#define ENUM_CONST_REG_STR(enum_class, str) \
+ helper->register_enum(#str, enum_class::ENUM_CONST(str));
+
namespace dsn {
template <typename TEnum>
diff --git a/src/utils/metrics.h b/src/utils/metrics.h
index 789a5bac2..b474538e9 100644
--- a/src/utils/metrics.h
+++ b/src/utils/metrics.h
@@ -644,62 +644,65 @@ private:
// On the other hand, it is also needed when some special operation should be done
// for a metric type. For example, percentile should be closed while it's no longer
// used.
+#define ENUM_FOREACH_METRIC_TYPE(DEF) \
+ DEF(Gauge) \
+ DEF(Counter) \
+ DEF(VolatileCounter) \
+ DEF(Percentile)
+
enum class metric_type
{
- kGauge,
- kCounter,
- kVolatileCounter,
- kPercentile,
- kInvalidUnit,
+ ENUM_FOREACH_METRIC_TYPE(ENUM_CONST_DEF) kInvalidType,
};
-ENUM_BEGIN(metric_type, metric_type::kInvalidUnit)
-ENUM_REG_WITH_CUSTOM_NAME(metric_type::kGauge, gauge)
-ENUM_REG_WITH_CUSTOM_NAME(metric_type::kCounter, counter)
-ENUM_REG_WITH_CUSTOM_NAME(metric_type::kVolatileCounter, volatile_counter)
-ENUM_REG_WITH_CUSTOM_NAME(metric_type::kPercentile, percentile)
+#define ENUM_CONST_REG_STR_METRIC_TYPE(str) ENUM_CONST_REG_STR(metric_type, str)
+
+ENUM_BEGIN(metric_type, metric_type::kInvalidType)
+ENUM_FOREACH_METRIC_TYPE(ENUM_CONST_REG_STR_METRIC_TYPE)
ENUM_END(metric_type)
+#define ENUM_FOREACH_METRIC_UNIT(DEF) \
+ DEF(NanoSeconds) \
+ DEF(MicroSeconds) \
+ DEF(MilliSeconds) \
+ DEF(Seconds) \
+ DEF(Bytes) \
+ DEF(MegaBytes) \
+ DEF(CapacityUnits) \
+ DEF(Percent) \
+ DEF(Replicas) \
+ DEF(Partitions) \
+ DEF(PartitionSplittings) \
+ DEF(Servers) \
+ DEF(Requests) \
+ DEF(Responses) \
+ DEF(Seeks) \
+ DEF(PointLookups) \
+ DEF(Values) \
+ DEF(Keys) \
+ DEF(Files) \
+ DEF(Dirs) \
+ DEF(Amplification) \
+ DEF(Checkpoints) \
+ DEF(Flushes) \
+ DEF(Compactions) \
+ DEF(Mutations) \
+ DEF(Writes) \
+ DEF(Changes) \
+ DEF(Operations) \
+ DEF(Tasks) \
+ DEF(Disconnections) \
+ DEF(Learns) \
+ DEF(Rounds) \
+ DEF(Resets) \
+ DEF(Backups) \
+ DEF(FileLoads) \
+ DEF(FileUploads) \
+ DEF(BulkLoads)
+
enum class metric_unit : size_t
{
- kNanoSeconds,
- kMicroSeconds,
- kMilliSeconds,
- kSeconds,
- kBytes,
- kMegaBytes,
- kCapacityUnits,
- kPercent,
- kReplicas,
- kPartitions,
- kPartitionSplittings,
- kServers,
- kRequests,
- kResponses,
- kSeeks,
- kPointLookups,
- kValues,
- kKeys,
- kFiles,
- kDirs,
- kAmplification,
- kCheckpoints,
- kFlushes,
- kCompactions,
- kMutations,
- kWrites,
- kChanges,
- kOperations,
- kTasks,
- kDisconnections,
- kLearns,
- kRounds,
- kResets,
- kBackups,
- kFileLoads,
- kFileUploads,
- kBulkLoads,
- kInvalidUnit,
+ ENUM_FOREACH_METRIC_UNIT(ENUM_CONST_DEF) kInvalidUnit,
};
#define METRIC_ASSERT_UNIT_LATENCY(unit, index) \
@@ -727,12 +730,10 @@ inline uint64_t convert_metric_latency_from_ns(uint64_t latency_ns, metric_unit
return latency_ns / kMetricLatencyConverterFromNS[index];
}
+#define ENUM_CONST_REG_STR_METRIC_UNIT(str) ENUM_CONST_REG_STR(metric_unit, str)
+
ENUM_BEGIN(metric_unit, metric_unit::kInvalidUnit)
-ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kNanoSeconds, nanoseconds)
-ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kMicroSeconds, microseconds)
-ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kMilliSeconds, milliseconds)
-ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kSeconds, seconds)
-ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kRequests, requests)
+ENUM_FOREACH_METRIC_UNIT(ENUM_CONST_REG_STR_METRIC_UNIT)
ENUM_END(metric_unit)
class metric_prototype
diff --git a/src/utils/test/enum_helper_test.cpp b/src/utils/test/enum_helper_test.cpp
new file mode 100644
index 000000000..ec2472ed2
--- /dev/null
+++ b/src/utils/test/enum_helper_test.cpp
@@ -0,0 +1,212 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "utils/enum_helper.h"
+
+// IWYU pragma: no_include <gtest/gtest-message.h>
+// IWYU pragma: no_include <gtest/gtest-param-test.h>
+// IWYU pragma: no_include <gtest/gtest-test-part.h>
+#include <gtest/gtest.h>
+#include <vector>
+
+namespace dsn {
+
+enum class command_type
+{
+ START,
+ RESTART,
+ STOP,
+ COUNT,
+ INVALID_TYPE,
+};
+
+ENUM_BEGIN(command_type, command_type::INVALID_TYPE)
+ENUM_REG2(command_type, START)
+ENUM_REG_WITH_CUSTOM_NAME(command_type::RESTART, restart)
+ENUM_REG(command_type::STOP)
+ENUM_END(command_type)
+
+using command_type_enum_from_string_case = std::tuple<std::string, command_type>;
+
+class CommandTypeEnumFromStringTest
+ : public testing::TestWithParam<command_type_enum_from_string_case>
+{
+};
+
+TEST_P(CommandTypeEnumFromStringTest, EnumFromString)
+{
+ std::string str;
+ command_type expected_type;
+ std::tie(str, expected_type) = GetParam();
+
+ auto actual_type = enum_from_string(str.c_str(), command_type::INVALID_TYPE);
+ EXPECT_EQ(expected_type, actual_type);
+}
+
+const std::vector<command_type_enum_from_string_case> command_type_enum_from_string_tests = {
+ {"START", command_type::START},
+ {"Start", command_type::INVALID_TYPE},
+ {"start", command_type::INVALID_TYPE},
+ {"command_type::START", command_type::INVALID_TYPE},
+ {"command_type::Start", command_type::INVALID_TYPE},
+ {"command_type::start", command_type::INVALID_TYPE},
+ {"RESTART", command_type::INVALID_TYPE},
+ {"Restart", command_type::INVALID_TYPE},
+ {"restart", command_type::RESTART},
+ {"command_type::RESTART", command_type::INVALID_TYPE},
+ {"command_type::Restart", command_type::INVALID_TYPE},
+ {"command_type::restart", command_type::INVALID_TYPE},
+ {"STOP", command_type::INVALID_TYPE},
+ {"Stop", command_type::INVALID_TYPE},
+ {"stop", command_type::INVALID_TYPE},
+ {"command_type::STOP", command_type::STOP},
+ {"command_type::Stop", command_type::INVALID_TYPE},
+ {"command_type::stop", command_type::INVALID_TYPE},
+ {"COUNT", command_type::INVALID_TYPE}, // Since COUNT was not registered with specified string
+ {"UNDEFINE_TYPE", command_type::INVALID_TYPE},
+};
+
+INSTANTIATE_TEST_CASE_P(EnumHelperTest,
+ CommandTypeEnumFromStringTest,
+ testing::ValuesIn(command_type_enum_from_string_tests));
+
+using command_type_enum_to_string_case = std::tuple<command_type, std::string>;
+
+class CommandTypeEnumToStringTest : public testing::TestWithParam<command_type_enum_to_string_case>
+{
+};
+
+TEST_P(CommandTypeEnumToStringTest, EnumToString)
+{
+ command_type type;
+ std::string expected_str;
+ std::tie(type, expected_str) = GetParam();
+
+ std::string actual_str(enum_to_string(type));
+ EXPECT_EQ(expected_str, actual_str);
+}
+
+const std::vector<command_type_enum_to_string_case> command_type_enum_to_string_tests = {
+ {command_type::START, "START"},
+ {command_type::RESTART, "restart"},
+ {command_type::STOP, "command_type::STOP"},
+ {command_type::COUNT, "Unknown"}, // Since COUNT was not registered with specified string
+ {command_type::INVALID_TYPE, "Unknown"},
+};
+
+INSTANTIATE_TEST_CASE_P(EnumHelperTest,
+ CommandTypeEnumToStringTest,
+ testing::ValuesIn(command_type_enum_to_string_tests));
+
+#define ENUM_FOREACH_STATUS_CODE(DEF) \
+ DEF(Ok) \
+ DEF(NotFound) \
+ DEF(Corruption) \
+ DEF(IOError)
+
+enum class status_code
+{
+ ENUM_FOREACH_STATUS_CODE(ENUM_CONST_DEF) kCount,
+ kInvalidCode,
+};
+
+#define ENUM_CONST_REG_STR_STATUS_CODE(str) ENUM_CONST_REG_STR(status_code, str)
+
+ENUM_BEGIN(status_code, status_code::kInvalidCode)
+ENUM_FOREACH_STATUS_CODE(ENUM_CONST_REG_STR_STATUS_CODE)
+ENUM_END(status_code)
+
+using status_code_enum_from_string_case = std::tuple<std::string, status_code>;
+
+class StatusCodeEnumFromStringTest
+ : public testing::TestWithParam<status_code_enum_from_string_case>
+{
+};
+
+TEST_P(StatusCodeEnumFromStringTest, EnumFromString)
+{
+ std::string str;
+ status_code expected_code;
+ std::tie(str, expected_code) = GetParam();
+
+ auto actual_code = enum_from_string(str.c_str(), status_code::kInvalidCode);
+ EXPECT_EQ(expected_code, actual_code);
+}
+
+const std::vector<status_code_enum_from_string_case> status_code_enum_from_string_tests = {
+ {"OK", status_code::kInvalidCode},
+ {"Ok", status_code::kOk},
+ {"ok", status_code::kInvalidCode},
+ {"status_code::OK", status_code::kInvalidCode},
+ {"status_code::Ok", status_code::kInvalidCode},
+ {"status_code::ok", status_code::kInvalidCode},
+ {"NOTFOUND", status_code::kInvalidCode},
+ {"NotFound", status_code::kNotFound},
+ {"notfound", status_code::kInvalidCode},
+ {"status_code::NOTFOUND", status_code::kInvalidCode},
+ {"status_code::NotFound", status_code::kInvalidCode},
+ {"status_code::notfound", status_code::kInvalidCode},
+ {"CORRUPTION", status_code::kInvalidCode},
+ {"Corruption", status_code::kCorruption},
+ {"corruption", status_code::kInvalidCode},
+ {"status_code::CORRUPTION", status_code::kInvalidCode},
+ {"status_code::Corruption", status_code::kInvalidCode},
+ {"status_code::corruption", status_code::kInvalidCode},
+ {"IOERROR", status_code::kInvalidCode},
+ {"IOError", status_code::kIOError},
+ {"ioerror", status_code::kInvalidCode},
+ {"status_code::IOERROR", status_code::kInvalidCode},
+ {"status_code::IOError", status_code::kInvalidCode},
+ {"status_code::ioerror", status_code::kInvalidCode},
+ {"Count", status_code::kInvalidCode}, // Since kCount was not registered with specified string
+ {"UndefinedCode", status_code::kInvalidCode},
+};
+
+INSTANTIATE_TEST_CASE_P(EnumHelperTest,
+ StatusCodeEnumFromStringTest,
+ testing::ValuesIn(status_code_enum_from_string_tests));
+
+using status_code_enum_to_string_case = std::tuple<status_code, std::string>;
+
+class StatusCodeEnumToStringTest : public testing::TestWithParam<status_code_enum_to_string_case>
+{
+};
+
+TEST_P(StatusCodeEnumToStringTest, EnumToString)
+{
+ status_code code;
+ std::string expected_str;
+ std::tie(code, expected_str) = GetParam();
+
+ std::string actual_str(enum_to_string(code));
+ EXPECT_EQ(expected_str, actual_str);
+}
+
+const std::vector<status_code_enum_to_string_case> status_code_enum_to_string_tests = {
+ {status_code::kOk, "Ok"},
+ {status_code::kNotFound, "NotFound"},
+ {status_code::kCorruption, "Corruption"},
+ {status_code::kIOError, "IOError"},
+ {status_code::kCount, "Unknown"}, // Since kCount was not registered with specified string
+ {status_code::kInvalidCode, "Unknown"},
+};
+
+INSTANTIATE_TEST_CASE_P(EnumHelperTest,
+ StatusCodeEnumToStringTest,
+ testing::ValuesIn(status_code_enum_to_string_tests));
+
+} // namespace dsn
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pegasus.apache.org
For additional commands, e-mail: commits-help@pegasus.apache.org