Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hotkey): add thread_safe method to clear collector #640

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

Smityz
Copy link
Contributor

@Smityz Smityz commented Nov 7, 2020

No description provided.

Co-authored-by: Yingchun Lai <405403881@qq.com>
@@ -332,5 +339,12 @@ void hotkey_fine_data_collector::analyse_data(detect_hotkey_result &result)
}
}

void hotkey_fine_data_collector::clear()
{
std::pair<dsn::blob, uint64_t> key_weight_pair;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about change it like this?

    moodycamel::ConcurrentQueue<std::pair<dsn::blob, uint64_t>> temp;
    _capture_key_queue.swap(temp);

Copy link
Contributor Author

@Smityz Smityz Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concurrentqueue.h:890
	// Swaps this queue's state with the other's. Not thread-safe.
	// Swapping two queues does not invalidate their tokens, however
	// the tokens that were created for one queue must be used with
	// only the swapped queue (i.e. the tokens are tied to the
	// queue's movable state, not the object itself).

It's not a thread safe way

@@ -102,6 +102,7 @@ class internal_collector_base : public dsn::replication::replica_base
explicit internal_collector_base(replica_base *base) : replica_base(base){};
virtual void capture_data(const dsn::blob &hash_key, uint64_t weight) = 0;
virtual void analyse_data(detect_hotkey_result &result) = 0;
virtual void clear() = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where you call this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Smityz Smityz requested a review from levy5307 November 10, 2020 03:33
@neverchanje neverchanje merged commit 7011500 into apache:master Nov 10, 2020
@neverchanje neverchanje mentioned this pull request Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants