You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2020/10/19 02:25:17 UTC
[GitHub] [incubator-pegasus] hycdong commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
hycdong commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r507357822
##########
File path: src/server/hotkey_collector.h
##########
@@ -63,10 +63,11 @@ class internal_collector_base;
// | | | Hotkey |
// +--------------------+ +----------------------------------------------------+
-class hotkey_collector
+class hotkey_collector : public dsn::replication::replica_base
Review comment:
Is it necessary to inherit `replica_base` class? Will hotkey_collector use other functions except log-printing functions like `ddebug_replica`?
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,39 @@
#include "hotkey_collector.h"
+#include <dsn/dist/replication/replication_enums.h>
#include <dsn/utility/smart_pointers.h>
#include "base/pegasus_key_schema.h"
+#include <dsn/dist/fmt_logging.h>
namespace pegasus {
namespace server {
-hotkey_collector::hotkey_collector()
- : _internal_collector(dsn::make_unique<hotkey_empty_data_collector>())
+hotkey_collector::hotkey_collector(dsn::replication::hotkey_type::type hotkey_type,
+ dsn::replication::replica_base *r_base)
+ : replica_base(r_base),
+ _state(hotkey_collector_state::STOPPED),
+ _hotkey_type(hotkey_type),
+ _internal_collector(std::make_shared<hotkey_empty_data_collector>())
{
}
-// TODO: (Tangyanzhao) implement these functions
void hotkey_collector::handle_rpc(const dsn::replication::detect_hotkey_request &req,
dsn::replication::detect_hotkey_response &resp)
{
+ switch (req.action) {
+ case dsn::replication::detect_action::START:
+ on_start_detect(resp);
+ return;
+ case dsn::replication::detect_action::STOP:
+ on_stop_detect(resp);
+ return;
+ default:
+ std::string hint = fmt::format("{}: can't find this detect action", req.action);
+ resp.err = dsn::ERR_INVALID_VERSION;
+ resp.__set_err_hint(hint);
+ dwarn_replica(hint);
Review comment:
It is better to use error level log here.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +67,51 @@ void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weigh
void hotkey_collector::analyse_data() { _internal_collector->analyse_data(); }
+void hotkey_collector::on_start_detect(dsn::replication::detect_hotkey_response &resp)
+{
+ auto now_state = _state.load();
+ std::string hint;
+ switch (now_state) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ resp.err = dsn::ERR_SERVICE_ALREADY_EXIST;
+ hint = fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(now_state));
+ ddebug_replica(hint);
Review comment:
I think warning or error level may be better, the following log-printing also has this problem.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +67,51 @@ void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weigh
void hotkey_collector::analyse_data() { _internal_collector->analyse_data(); }
+void hotkey_collector::on_start_detect(dsn::replication::detect_hotkey_response &resp)
+{
+ auto now_state = _state.load();
+ std::string hint;
+ switch (now_state) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ resp.err = dsn::ERR_SERVICE_ALREADY_EXIST;
+ hint = fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(now_state));
+ ddebug_replica(hint);
+ return;
+ case hotkey_collector_state::FINISHED:
+ resp.err = dsn::ERR_SERVICE_ALREADY_EXIST;
+ hint = fmt::format(
+ "{} hotkey result has been found, you can send a stop rpc to restart hotkey detection",
+ dsn::enum_to_string(_hotkey_type));
+ ddebug_replica(hint);
+ return;
+ case hotkey_collector_state::STOPPED:
+ // TODO: (Tangyanzhao) start coarse detecting
+ _state.store(hotkey_collector_state::COARSE_DETECTING);
+ resp.err = dsn::ERR_OK;
+ hint = fmt::format("starting to detect {} hotkey", dsn::enum_to_string(_hotkey_type));
+ ddebug_replica(hint);
+ return;
+ default:
+ hint = "invalid collector state";
+ resp.err = dsn::ERR_INVALID_VERSION;
+ resp.__set_err_hint(hint);
+ dwarn_replica(hint);
+ dassert(false, "invalid collector state");
+ }
+}
+
+void hotkey_collector::on_stop_detect(dsn::replication::detect_hotkey_response &resp)
+{
+ _state.store(hotkey_collector_state::STOPPED);
Review comment:
There are no state check here, it means detection can be stopped in any states.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +67,51 @@ void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weigh
void hotkey_collector::analyse_data() { _internal_collector->analyse_data(); }
+void hotkey_collector::on_start_detect(dsn::replication::detect_hotkey_response &resp)
+{
+ auto now_state = _state.load();
+ std::string hint;
+ switch (now_state) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ resp.err = dsn::ERR_SERVICE_ALREADY_EXIST;
Review comment:
Maybe `ERR_INVALID_STATE` is better, if you think ERR_INVALID_STATE is not suitable to show the error, you can define a new error_code.
The error_code problem is also in other cases in this switch condition.
Besides, I wonder what will rpc client act if it receives this error?
----------------------------------------------------------------
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