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/11/18 06:39:06 UTC
[GitHub] [incubator-pegasus] Smityz opened a new pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Smityz opened a new pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632
This PR is to solve two hidden dangers in hotkey_collector.
Because the related codes of two dangers are overlapped, I solve them in one PR.
The first hidden danger is if `FLAGS_xxx` variable is dynamically changing in the future, `FLAGS_data_capture_hash_bucket_num` might be changing when capturing data, which will cause the memory overflow.
The second hidden danger is `_internal_collector` might be reset while other threads are using 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
[GitHub] [incubator-pegasus] hycdong commented on pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
hycdong commented on pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#issuecomment-718329292
> The second hidden danger is _internal_collector might be reset while other threads are using it.
I don't understand what is the hidden danger of it, and how you fix it, maybe you can add more description about 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
[GitHub] [incubator-pegasus] acelyc111 commented on a change in pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r528331026
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -145,19 +202,24 @@ 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)
{
// TODO: (Tangyanzhao) add a unit test to ensure data integrity
- _internal_collector->capture_data(hash_key, weight);
+ switch (_state.load()) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ _internal_coarse_collector->capture_data(hash_key, weight > 0 ? weight : 1);
Review comment:
Why the two states both use coarse 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] Smityz commented on a change in pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r514142516
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -142,17 +139,19 @@ 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)
{
// TODO: (Tangyanzhao) add a unit test to ensure data integrity
- _internal_collector->capture_data(hash_key, weight);
+ if (_state.load() == hotkey_collector_state::COARSE_DETECTING &&
Review comment:
`load()` will lose performance, find a way to delete it.
And check the new way in santilizer
----------------------------------------------------------------
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 #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r528459562
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -145,19 +202,24 @@ 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)
{
// TODO: (Tangyanzhao) add a unit test to ensure data integrity
- _internal_collector->capture_data(hash_key, weight);
+ switch (_state.load()) {
+ case hotkey_collector_state::COARSE_DETECTING:
+ case hotkey_collector_state::FINE_DETECTING:
+ _internal_coarse_collector->capture_data(hash_key, weight > 0 ? weight : 1);
Review comment:
fiexd
----------------------------------------------------------------
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 edited a comment on pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
hycdong edited a comment on pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#issuecomment-718329292
> The second hidden danger is _internal_collector might be reset while other threads are using it.
I don't understand what is the hidden danger of it, and how you fix it, maybe you can add more description about 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
[GitHub] [incubator-pegasus] levy5307 commented on a change in pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
levy5307 commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r513244162
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -162,6 +162,14 @@ void hotkey_collector::analyse_data()
}
}
+inline void hotkey_collector::init_internal_collector()
Review comment:
I think `reset_internal_collector` is 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
[GitHub] [incubator-pegasus] Smityz commented on a change in pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r514142516
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -142,17 +139,19 @@ 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)
{
// TODO: (Tangyanzhao) add a unit test to ensure data integrity
- _internal_collector->capture_data(hash_key, weight);
+ if (_state.load() == hotkey_collector_state::COARSE_DETECTING &&
Review comment:
`load()` will lose performance, find a way to delete 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
[GitHub] [incubator-pegasus] Shuo-Jia commented on a change in pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r522764624
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -102,20 +101,78 @@ find_outlier_index(const std::vector<uint64_t> &captured_keys, int threshold, in
}
// TODO: (Tangyanzhao) replace it to xxhash
-static int get_bucket_id(dsn::string_view data)
+static int get_bucket_id(dsn::string_view data, int bucket_num)
{
- size_t hash_value = boost::hash_range(data.begin(), data.end());
- return static_cast<int>(hash_value % FLAGS_hotkey_buckets_num);
+ return static_cast<int>(boost::hash_range(data.begin(), data.end()) % bucket_num);
}
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>(this)),
- _collector_start_time_second(0)
+ : replica_base(r_base), _hotkey_type(hotkey_type)
{
+ int now_hash_bucket_num = FLAGS_hotkey_buckets_num;
+ _internal_coarse_collector =
+ std::make_shared<hotkey_coarse_data_collector>(this, now_hash_bucket_num);
+ _internal_fine_collector =
+ std::make_shared<hotkey_fine_data_collector>(this, now_hash_bucket_num);
+ _internal_empty_collector = std::make_shared<hotkey_empty_data_collector>(this);
+ hotkey_collector::change_state_to_stopped();
Review comment:
is the line necessary? At the beginning of the `hotkey_collector`, the all varible should have been default value.
----------------------------------------------------------------
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 #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
acelyc111 merged pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632
----------------------------------------------------------------
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 closed pull request #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz closed pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632
----------------------------------------------------------------
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 #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
hycdong commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r513901372
##########
File path: src/server/hotkey_collector.h
##########
@@ -115,11 +118,13 @@ class hotkey_empty_data_collector : public internal_collector_base
class hotkey_coarse_data_collector : public internal_collector_base
{
public:
- explicit hotkey_coarse_data_collector(replica_base *base);
+ hotkey_coarse_data_collector() = delete;
+ explicit hotkey_coarse_data_collector(replica_base *base, int data_capture_hash_bucket_num);
Review comment:
hotkey_fine_data_collector constructor function also should add parameter `data_capture_hash_bucket_num`, you can add TODO or remember it after the pull request 631 is merged.
----------------------------------------------------------------
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 #632: fix(hotkey): fix two hidden dangers in hotkey_collector
Posted by GitBox <gi...@apache.org>.
Smityz commented on a change in pull request #632:
URL: https://github.com/apache/incubator-pegasus/pull/632#discussion_r514142516
##########
File path: src/server/hotkey_collector.cpp
##########
@@ -142,17 +139,19 @@ 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)
{
// TODO: (Tangyanzhao) add a unit test to ensure data integrity
- _internal_collector->capture_data(hash_key, weight);
+ if (_state.load() == hotkey_collector_state::COARSE_DETECTING &&
Review comment:
`load()` will lose performance, find a way to delete it.
And check the new way in [ThreadSanitizer](https://godbolt.org/)
----------------------------------------------------------------
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