[GitHub] [incubator-pegasus] hycdong commented on a change in pull request #597: refactor(collector): sort out the structure of partition hotspot detection

[GitBox <gi...@apache.org>]

hycdong commented on a change in pull request #597:
URL: https://github.com/apache/incubator-pegasus/pull/597#discussion_r484667532



##########
File path: src/server/info_collector.h
##########
@@ -177,15 +177,14 @@ class info_collector
     uint32_t _storage_size_fetch_interval_seconds;
     uint32_t _storage_size_retry_wait_seconds;
     uint32_t _storage_size_retry_max_count;
-    std::string _hotspot_detect_algorithm;
     ::dsn::task_ptr _storage_size_stat_timer_task;
     ::dsn::utils::ex_lock_nr _capacity_unit_update_info_lock;
     // mapping 'node address' --> 'last updated timestamp'
     std::map<std::string, string> _capacity_unit_update_info;
-    std::map<std::string, hotspot_calculator *> _hotspot_calculator_store;
+    std::map<std::string, hotspot_partition_calculator *> _hotspot_calculator_store;

Review comment:
       I think raw pointer is dangerous, I suggest using smart pointer instead.
   Besides, adding comment for `_hotspot_calculator_store`.

##########
File path: src/server/hotspot_partition_calculator.h
##########
@@ -0,0 +1,52 @@
+// 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.
+
+#pragma once
+
+#include "hotspot_partition_data.h"
+#include <gtest/gtest_prod.h>
+#include <dsn/perf_counter/perf_counter.h>
+
+namespace pegasus {
+namespace server {
+
+// hotspot_partition_calculator is used to find the hotspot in Pegasus
+class hotspot_partition_calculator
+{
+public:
+    hotspot_partition_calculator(const std::string &app_name, const int partition_num)

Review comment:
       I suggest adding comments for `_points`, `_app_data`, function `start_alg` and `aggregate`.
   Besides, adding comments to explain why `kMaxQueueSize = 100`.

##########
File path: src/server/info_collector.cpp
##########
@@ -150,15 +146,12 @@ void info_collector::on_app_stat()
         // get row data statistics for all of the apps
         all_stats.merge(app_stats);
 
-        // hotspot_calculator is to detect hotspots
-        hotspot_calculator *hotspot_calculator =
+        // hotspot_partition_calculator is to detect hotspots
+        hotspot_partition_calculator *hotspot_partition_calculator =
             get_hotspot_calculator(app_rows.first, app_rows.second.size());
-        if (!hotspot_calculator) {
-            continue;
-        }
-        hotspot_calculator->aggregate(app_rows.second);
-        // new policy can be designed by strategy pattern in hotspot_partition_data.h
-        hotspot_calculator->start_alg();
+        dassert(hotspot_partition_calculator != nullptr, "hotspot_partition_calculator is NULL");

Review comment:
       I suggest printing more information such as `app_name`, `partition_count` in assert.
   Besides, is it have to assert here? If the hotspot_partition_calculator is not found, `on_app_stat` can also work.

##########
File path: src/server/info_collector.cpp
##########
@@ -302,25 +295,17 @@ void info_collector::on_storage_size_stat(int remaining_retry_count)
     _result_writer->set_result(st_stat.timestamp, "ss", st_stat.dump_to_json());
 }
 
-hotspot_calculator *info_collector::get_hotspot_calculator(const std::string &app_name,
-                                                           const int partition_num)
+hotspot_partition_calculator *info_collector::get_hotspot_calculator(const std::string &app_name,
+                                                                     const int partition_num)

Review comment:
       `partition_count` may be better




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org