acelyc111 commented on code in PR #2085:
URL: 
https://github.com/apache/incubator-pegasus/pull/2085#discussion_r1704341950


##########
src/shell/command_helper.h:
##########
@@ -709,30 +709,68 @@ inline std::vector<dsn::http_result> get_metrics(const 
std::vector<node_desc> &n
     return results;
 }
 
+// Adapt the result returned by `get_metrics` into the structure that could be 
processed by
+// `remote_command`.
+template <typename... Args>
+inline std::pair<bool, std::string> process_get_metrics_result(const 
dsn::http_result &result,

Review Comment:
   It seems the return type can be replaced by `error_s` or `absl::Status`.



##########
src/shell/main.cpp:
##########
@@ -101,7 +101,7 @@ static command_executor commands[] = {
         "get the node status for this cluster",
         "[-d|--detailed] [-j|--json] [-r|--resolve_ip] [-u|--resource_usage] "
         "[-o|--output file_name] [-s|--status all|alive|unalive] [-q|--qps] "
-        "[-t|--sample_interval_ms num]",
+        "[-i|--sample_interval_ms num]",

Review Comment:
   It should be mentioned in "Behavior changes" in the PR description for this 
PR.



##########
src/shell/commands/node_management.cpp:
##########
@@ -232,6 +234,160 @@ dsn::metric_filters rw_requests_filters()
     return filters;
 }
 
+dsn::metric_filters server_stat_filters()
+{
+    dsn::metric_filters filters;
+    filters.with_metric_fields = {dsn::kMetricNameField, 
dsn::kMetricSingleValueField};
+    filters.entity_types = {"server"};
+    filters.entity_metrics = {"virtual_mem_usage_mb", "resident_mem_usage_mb"};
+    return filters;
+}
+
+struct meta_server_stats
+{
+    meta_server_stats() = default;
+
+    double virt_mem_mb{0.0};
+    double res_mem_mb{0.0};
+
+    DEFINE_JSON_SERIALIZATION(virt_mem_mb, res_mem_mb)
+};
+
+std::pair<bool, std::string>
+aggregate_meta_server_stats(const node_desc &node,
+                            const dsn::metric_query_brief_value_snapshot 
&query_snapshot)
+{
+    aggregate_stats_calcs calcs;
+    meta_server_stats stats;
+    calcs.create_assignments<total_aggregate_stats>(
+        "server",
+        stat_var_map({{"virtual_mem_usage_mb", &stats.virt_mem_mb},
+                      {"resident_mem_usage_mb", &stats.res_mem_mb}}));
+
+    auto command_result = process_parse_metrics_result(
+        calcs.aggregate_metrics(query_snapshot), node, "aggregate meta server 
stats");
+    if (!command_result.first) {
+        // Metrics failed to be aggregated.
+        return command_result;
+    }
+
+    return std::make_pair(true,
+                          
dsn::json::json_forwarder<meta_server_stats>::encode(stats).to_string());
+}
+
+struct replica_server_stats

Review Comment:
   It's the same to `meta_server_stats`, how about using a unify one?



##########
src/shell/commands/node_management.cpp:
##########
@@ -232,6 +234,160 @@ dsn::metric_filters rw_requests_filters()
     return filters;
 }
 
+dsn::metric_filters server_stat_filters()
+{
+    dsn::metric_filters filters;
+    filters.with_metric_fields = {dsn::kMetricNameField, 
dsn::kMetricSingleValueField};
+    filters.entity_types = {"server"};
+    filters.entity_metrics = {"virtual_mem_usage_mb", "resident_mem_usage_mb"};

Review Comment:
   What about defining some static const strings for these metric names?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to