Add data structures, interface, and basic unit tests for hotkey sampl… #123

kevyang · 2017-01-21T01:04:01Z

…ing/detection

thinkingfish

I've reviewed the data structure not the tests.

src/hotkey/counter_table.c

+    STAILQ_ENTRY(counter_table_entry) next; /* entry in hash table or pool */
+
+    char                              key[MAX_KEY_LEN];
+    uint32_t                          nkey;


src/hotkey/counter_table.c

@@ -0,0 +1,242 @@
+#include "counter_table.h"


src/hotkey/counter_table.c

+}
+
+uint32_t
+counter_table_incr(char *key, uint32_t nkey)


src/hotkey/hotkey.h

+
+#define HOTKEY_NSAMPLE   10000 /* keep last 10000 keys sampled by default */
+#define HOTKEY_RATE      100   /* sample one in every 100 keys by default */
+#define HOTKEY_THRESHOLD 10    /* signal for hotkey if 10 or more keys in sample by default */


src/hotkey/hotkey.h

+
+/* TODO(kevyang): add stats for hotkey module */
+
+#define HOTKEY_NSAMPLE   10000 /* keep last 10000 keys sampled by default */


src/hotkey/hotkey.h

+
+#define hotkey_sample(key, nkey) do { \
+    if (hotkey_enabled) {                            \
+        _hotkey_sample(key, nkey);                   \


src/hotkey/queue.c

@@ -0,0 +1,190 @@
+#include "queue.h"


kevyang · 2017-02-17T20:41:18Z

src/hotkey/queue.c

+static bool qnp_init = false;
+
+struct queue_node {
+    char                     key[MAX_KEY_LEN];


I will do this in a separate pull request. This is a larger change than it seems, as in order to do this we need to make the queue aware of the counting map

seppo0010 · 2017-01-24T11:18:51Z

src/hotkey/counter_table.c

+    struct counter_table_entry *cte, *tcte;
+
+    if (!ctep_init) {
+        log_warn("counter_table_entry pool was not created, ignore");


missing a return;?

seppo0010 · 2017-01-24T11:27:00Z

src/hotkey/hotkey.c

+    log_info("Tear down the %s module", HOTKEY_MODULE_NAME);
+
+    if (!hotkey_init) {
+        log_warn("%s was not setup", HOTKEY_MODULE_NAME);


missing return;?

seppo0010 · 2017-01-24T11:33:42Z

src/hotkey/queue.c

+    struct queue_node *qn, *tqn;
+
+    if (!qnp_init) {
+        log_warn("queue_node pool was not created, ignore");


seppo0010 · 2017-01-24T11:35:16Z

src/hotkey/queue.c

+    log_info("tear down the %s module", QUEUE_MODULE_NAME);
+
+    if (!queue_init) {
+        log_warn("%s was not setup", QUEUE_MODULE_NAME);


seppo0010 · 2017-01-24T11:39:28Z

test/hotkey/counter_table/check_counter_table.c

+START_TEST(test_basic)
+{
+    char *key1 = "key1", *key2 = "key2";
+    uint32_t klen = 4;


Use two keys with different length. Also, since the char* is actually 5 bytes as it has a NULL termination, use 5 instead of 4 in this case, as it may help catch off-by-one errors.

I made the sizes the length of the strings without null termination, because we don't use null terminated keys in cache.

seppo0010 · 2017-01-24T11:41:49Z

src/hotkey/queue.c

+queue_pop(char *buf)
+{
+    struct queue_node *qn = STAILQ_FIRST(&q);
+    uint32_t nkey;


ASSERT(queue_size);?

seppo0010 · 2017-01-24T11:43:17Z

test/hotkey/queue/check_queue.c

+START_TEST(test_multiple)
+{
+    char *key1 = "key1", *key2 = "key22", *key3 = "key333";
+    uint32_t klen1 = 4, klen2 = 5, klen3 = 6;


5, 6 and 7?

kevyang · 2017-02-22T21:24:22Z

src/hotkey/key_window.h

+#include <stdint.h>
+
+/*
+ * The key_window module provides a FIFO key_window interface to help with bookkeeping the


I think the benefits of using ring_array do not significantly outweigh the drawbacks. Although it would provide a clean push/pop, using STAILQ is also a one liner in this aspect and is not significantly harder to understand. ring_array also lacks a foreach equivalent, which means we would need to write a messier loop for freeing the contents. Additionally, using ring_array still does not remove the need to check whether the the window is full. Furthermore, if we are to port this hotkey change to twemcache, twemcache already has mc_queue so it would be mostly read to go; however, were we to switch to ring_array we would need to port ring_array over to twemcache as well. For these reasons, I think we should keep it as it is for now.

test/hotkey/key_window/check_key_window.c

+
+START_TEST(test_multiple)
+{
+    char *key1str = "key1", *key2str = "key22", *key3str = "key333";


Add data structures, interface, and basic unit tests for hotkey sampl…

e3d97fc

…ing/detection

thinkingfish suggested changes Jan 24, 2017

View reviewed changes

seppo0010 reviewed Jan 24, 2017

View reviewed changes

address comments

a8ecabb

thinkingfish suggested changes Feb 19, 2017

View reviewed changes

fix test keys

4d15b8b

thinkingfish approved these changes Feb 23, 2017

View reviewed changes

kevyang merged commit 0d32128 into twitter:master Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data structures, interface, and basic unit tests for hotkey sampl… #123

Add data structures, interface, and basic unit tests for hotkey sampl… #123

kevyang commented Jan 21, 2017

thinkingfish left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kevyang Feb 17, 2017

seppo0010 Jan 24, 2017

seppo0010 Jan 24, 2017

seppo0010 Jan 24, 2017

seppo0010 Jan 24, 2017

seppo0010 Jan 24, 2017

kevyang Feb 17, 2017

seppo0010 Jan 24, 2017

seppo0010 Jan 24, 2017

This comment was marked as spam.

kevyang Feb 22, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.


		/* TODO(kevyang): add stats for hotkey module */

		#define HOTKEY_NSAMPLE 10000 /* keep last 10000 keys sampled by default */

Add data structures, interface, and basic unit tests for hotkey sampl… #123

Add data structures, interface, and basic unit tests for hotkey sampl… #123

Conversation

kevyang commented Jan 21, 2017

thinkingfish left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.