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/15 07:11:47 UTC
[GitHub] [incubator-pegasus] Smityz opened a new pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Smityz opened a new pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621
implement the RPC handler in hotkey_collector
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] acelyc111 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506002202
##########
File path: src/server/hotkey_collector.h
##########
@@ -19,6 +19,8 @@
#include <dsn/utility/string_view.h>
#include <dsn/dist/replication/replication_types.h>
+#include <dsn/dist/replication/replica_base.h>
+#include <dsn/dist/fmt_logging.h>
Review comment:
Move this line to cpp file
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -16,14 +16,34 @@
// under the License.
#include "hotkey_collector.h"
+#include <dsn/dist/replication/replication_enums.h>
namespace pegasus {
namespace server {
-// TODO: (Tangyanzhao) implement these functions
+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)
+{
+}
+
void hotkey_collector::handle_rpc(const dsn::replication::detect_hotkey_request &req,
dsn::replication::detect_hotkey_response &resp)
{
+ if (req.action == dsn::replication::detect_action::START) {
Review comment:
Use `switch..case..`
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -33,5 +53,40 @@ void hotkey_collector::capture_raw_key(const dsn::blob &raw_key, int64_t weight)
void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weight) {}
+bool hotkey_collector::start_detect(std::string &err_hint)
+{
+ auto now_state = _state.load();
+ switch (_state.load()) {
Review comment:
```suggestion
switch (now_state) {
```
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -16,14 +16,34 @@
// under the License.
#include "hotkey_collector.h"
+#include <dsn/dist/replication/replication_enums.h>
namespace pegasus {
namespace server {
-// TODO: (Tangyanzhao) implement these functions
+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)
+{
+}
+
void hotkey_collector::handle_rpc(const dsn::replication::detect_hotkey_request &req,
dsn::replication::detect_hotkey_response &resp)
{
+ if (req.action == dsn::replication::detect_action::START) {
+ std::string err_hint;
+ if (start_detect(err_hint)) {
+ resp.err = dsn::ERR_OK;
Review comment:
You can move this line at first in this function, then it's not needed to reassign it again and again.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -33,5 +53,40 @@ void hotkey_collector::capture_raw_key(const dsn::blob &raw_key, int64_t weight)
void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weight) {}
+bool hotkey_collector::start_detect(std::string &err_hint)
+{
+ auto now_state = _state.load();
+ switch (_state.load()) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ err_hint = fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(now_state));
Review comment:
Unify to add or remove `dsn::`
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -33,5 +53,40 @@ void hotkey_collector::capture_raw_key(const dsn::blob &raw_key, int64_t weight)
void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weight) {}
+bool hotkey_collector::start_detect(std::string &err_hint)
+{
+ auto now_state = _state.load();
+ switch (_state.load()) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ err_hint = fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(now_state));
+ ddebug_replica(err_hint);
+ return false;
+ case hotkey_collector_state::FINISHED:
+ err_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(err_hint);
+ return false;
+ case hotkey_collector_state::STOPPED:
+ // TODO: (Tangyanzhao) start coarse detecting
+ _state.store(hotkey_collector_state::COARSE_DETECTING);
+ ddebug_replica("starting to detect {} hotkey", dsn::enum_to_string(_hotkey_type));
+ return true;
+ default:
+ err_hint = "invalid collector state";
+ ddebug_replica(err_hint);
Review comment:
It's not possible to reach here, I think you can assert here.
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506123021
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,53 @@
#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:
+ if (is_detecting_now()) {
Review comment:
According to this comment https://github.com/apache/incubator-pegasus/pull/621#discussion_r506004399, I combined these functions
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] hycdong commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [incubator-pegasus] levy5307 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506120879
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +81,36 @@ void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weigh
void hotkey_collector::analyse_data() { _internal_collector->analyse_data(); }
+bool hotkey_collector::is_detecting_now()
+{
+ return (_state.load() == hotkey_collector_state::COARSE_DETECTING ||
Review comment:
```
auto currnet_state = _state.load();
return (current_state == hotkey_collector_state::COARSE_DETECTING ||
current_state == hotkey_collector_state::FINE_DETECTING);
```
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] acelyc111 merged pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 merged pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] acelyc111 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506787974
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +66,50 @@ 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);
Review comment:
Better to assert here, coz the state is internal enum, default case will not happen.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,38 @@
#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:
+ resp.err = dsn::ERR_INVALID_VERSION;
+ resp.__set_err_hint("can't find this detect_action");
+ dwarn_replica("can't find this detect_action");
Review comment:
Add error details, i.e. `req.action`, into hint message and log, then admins know what happend.
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r507370795
##########
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:
Yes, it is a mandatory termination function
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] levy5307 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506118125
##########
File path: src/server/hotkey_collector.h
##########
@@ -76,8 +77,15 @@ class hotkey_collector
/*out*/ dsn::replication::detect_hotkey_response &resp);
private:
- std::unique_ptr<internal_collector_base> _internal_collector;
+ bool is_detecting_now();
Review comment:
inline
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] acelyc111 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506257276
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,37 @@
#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:
+ resp.err = dsn::ERR_INVALID_VERSION;
Review comment:
Add some log
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +65,50 @@ 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.c_str());
+ 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.c_str());
+ 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.c_str());
+ return;
+ default:
+ hint = "invalid collector state";
+ resp.err = dsn::ERR_INVALID_VERSION;
+ resp.__set_err_hint(hint.c_str());
+ ddebug_replica(hint.c_str());
Review comment:
```suggestion
dwarn_replica(hint.c_str());
```
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +65,50 @@ 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.c_str());
Review comment:
The same to other places.
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -49,5 +65,50 @@ 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.c_str());
Review comment:
```suggestion
ddebug_replica(hint);
```
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] levy5307 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506119799
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,53 @@
#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:
+ if (is_detecting_now()) {
+ return_err_resp(resp,
+ fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(_state.load())));
Review comment:
add return;
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] levy5307 commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506118912
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,53 @@
#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:
+ if (is_detecting_now()) {
Review comment:
you can encapsulate into a new function for each case.
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506095347
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -33,5 +53,40 @@ void hotkey_collector::capture_raw_key(const dsn::blob &raw_key, int64_t weight)
void hotkey_collector::capture_hash_key(const dsn::blob &hash_key, int64_t weight) {}
+bool hotkey_collector::start_detect(std::string &err_hint)
+{
+ auto now_state = _state.load();
+ switch (_state.load()) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ err_hint = fmt::format("still detecting {} hotkey, state is {}",
+ dsn::enum_to_string(_hotkey_type),
+ enum_to_string(now_state));
Review comment:
change the namespace may lead to compile failed
`dsn::replication::hotkey_type::type` and `pegasus::hotkey_collector_state` are defined in different namespaces
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r507369782
##########
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:
`hotkey_collector` will not use other functions related to `replica_base` without `ddebug_replica`
----------------------------------------------------------------
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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #621: feat(hotkey): implement the RPC handler in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #621:
URL: https://github.com/apache/incubator-pegasus/pull/621#discussion_r506840477
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -17,21 +17,38 @@
#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:
+ resp.err = dsn::ERR_INVALID_VERSION;
+ resp.__set_err_hint("can't find this detect_action");
+ dwarn_replica("can't find this detect_action");
Review comment:
if `req.action` is an unknown enum, I can't transform it into a string to log it.
----------------------------------------------------------------
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