hycdong commented on a change in pull request #641:
URL: https://github.com/apache/incubator-pegasus/pull/641#discussion_r521133715
##########
File path: src/server/info_collector.cpp
##########
@@ -316,7 +316,8 @@ info_collector::get_hotspot_calculator(const std::string
&app_name, const int pa
if (iter != _hotspot_calculator_store.end()) {
return iter->second;
}
- auto calculator = std::make_shared<hotspot_partition_calculator>(app_name,
partition_count);
+ auto calculator =
+ std::make_shared<hotspot_partition_calculator>(app_name,
partition_count, _shell_context);
Review comment:
I think ddl_client is necessary for `hotspot_partition_calculator`, you
can create a new ddl client instance for calculator, not share the
shell_context with info_collector.
##########
File path: src/server/info_collector.cpp
##########
@@ -57,9 +57,9 @@ info_collector::info_collector()
_cluster_name = dsn::replication::get_current_cluster_name();
- _shell_context.current_cluster_name = _cluster_name;
- _shell_context.meta_list = meta_servers;
- _shell_context.ddl_client.reset(new replication_ddl_client(meta_servers));
+ _shell_context->current_cluster_name = _cluster_name;
+ _shell_context->meta_list = meta_servers;
+ _shell_context->ddl_client.reset(new replication_ddl_client(meta_servers));
Review comment:
It seems that `_shell_context` is not initialized.
##########
File path: src/server/hotspot_partition_calculator.cpp
##########
@@ -180,44 +177,39 @@ void
hotspot_partition_calculator::detect_hotkey_in_hotpartition(int data_type)
const dsn::replication::detect_action::type action)
{
FAIL_POINT_INJECT_F("send_detect_hotkey_request", [](dsn::string_view) {});
- auto request = dsn::make_unique<dsn::replication::detect_hotkey_request>();
- request->type = hotkey_type;
- request->action = action;
+
+ dsn::replication::detect_hotkey_request req;
+ req.type = hotkey_type;
+ req.action = action;
+ dsn::replication::detect_hotkey_response resp;
+
ddebug_f("{} {} hotkey detection in {}.{}",
(action == dsn::replication::detect_action::STOP) ? "Stop" :
"Start",
(hotkey_type == dsn::replication::hotkey_type::WRITE) ? "write" :
"read",
app_name,
partition_index);
- dsn::rpc_address meta_server;
- meta_server.assign_group("meta-servers");
- std::vector<dsn::rpc_address> meta_servers;
- replica_helper::load_meta_servers(meta_servers);
- for (const auto &address : meta_servers) {
- meta_server.group_address()->add(address);
+
+ int app_id;
+ int partition_count;
+ std::vector<dsn::partition_configuration> partitions;
+ _shell_context->ddl_client->list_app(app_name, app_id, partition_count,
partitions);
+ auto target_address = partitions[partition_index].primary;
+
Review comment:
Move the debug log here, and add `target_address` in this log.
##########
File path: src/server/hotspot_partition_calculator.h
##########
@@ -37,7 +37,9 @@ typedef std::vector<std::array<dsn::perf_counter_wrapper, 2>>
hot_partition_coun
class hotspot_partition_calculator
{
public:
- hotspot_partition_calculator(const std::string &app_name, int
partition_count)
+ hotspot_partition_calculator(const std::string &app_name,
+ int partition_count,
+ std::shared_ptr<shell_context> context)
Review comment:
It seems that _shell_context is not initialized.
##########
File path: src/server/hotspot_partition_calculator.cpp
##########
@@ -180,44 +177,39 @@ void
hotspot_partition_calculator::detect_hotkey_in_hotpartition(int data_type)
const dsn::replication::detect_action::type action)
{
FAIL_POINT_INJECT_F("send_detect_hotkey_request", [](dsn::string_view) {});
- auto request = dsn::make_unique<dsn::replication::detect_hotkey_request>();
- request->type = hotkey_type;
- request->action = action;
+
+ dsn::replication::detect_hotkey_request req;
+ req.type = hotkey_type;
+ req.action = action;
+ dsn::replication::detect_hotkey_response resp;
+
ddebug_f("{} {} hotkey detection in {}.{}",
(action == dsn::replication::detect_action::STOP) ? "Stop" :
"Start",
(hotkey_type == dsn::replication::hotkey_type::WRITE) ? "write" :
"read",
app_name,
partition_index);
- dsn::rpc_address meta_server;
- meta_server.assign_group("meta-servers");
- std::vector<dsn::rpc_address> meta_servers;
- replica_helper::load_meta_servers(meta_servers);
- for (const auto &address : meta_servers) {
- meta_server.group_address()->add(address);
+
+ int app_id;
+ int partition_count;
+ std::vector<dsn::partition_configuration> partitions;
+ _shell_context->ddl_client->list_app(app_name, app_id, partition_count,
partitions);
+ auto target_address = partitions[partition_index].primary;
Review comment:
It seems that you don't validate the `partition_index`, will the caller
ensure that the `partition_index` is valid?
##########
File path: src/server/info_collector.cpp
##########
@@ -143,7 +143,7 @@ void info_collector::on_app_stat()
{
ddebug("start to stat apps");
std::map<std::string, std::vector<row_data>> all_rows;
- if (!get_app_partition_stat(&_shell_context, all_rows)) {
+ if (!get_app_partition_stat(_shell_context.get(), all_rows)) {
Review comment:
+1, and I recommend that raise another pull request to do it, including
update `_shell_context` to std::shared_ptr, and update functions like
`get_app_partition_stat`
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
