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

Support slowlog command #87

Merged
merged 5 commits into from
Aug 30, 2016
Merged

Conversation

doyoubi
Copy link
Contributor

@doyoubi doyoubi commented Aug 19, 2016

Support slowlog command.

@doyoubi doyoubi changed the title [WIP]slowlog slowlog Aug 25, 2016
@doyoubi doyoubi changed the title slowlog Support slowlog command Aug 25, 2016
@@ -623,7 +807,14 @@ int cmd_parse_req(struct command *cmd, struct mbuf *buf)
cmd_mark_fail(cmd, rep_forward_err);
return CORVUS_OK;
}
redis_data_free(&r->data);

if (!slowlog_enabled() || !slowlog_type_need_log(cmd)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

better readability: slowlog_enabled() && slowlog_type_need_log(cmd)

@maralla
Copy link
Contributor

maralla commented Aug 26, 2016

It seems that refcount in slowlog entry is useless.

@doyoubi
Copy link
Contributor Author

doyoubi commented Aug 26, 2016

When a thread is going to store a new slowlog_entry in its slowlog_queue, it may delete an old one if the queue is already full, but it has no idea whether there are some other threads using this old slowlog_entry.

@@ -550,3 +550,16 @@ int pos_to_str(struct pos_array *pos, char *str)
str[length] = '\0';
return CORVUS_OK;
}

size_t pos_to_str_with_limit(struct pos_array *pos, uint8_t *str, size_t limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

pos_to_str can be simplified with this function :

size_t len = pos_to_str_with_limit(pos, str, pos->str_len);
str[len] = '\0';
return CORVUS_OK;

@maralla
Copy link
Contributor

maralla commented Aug 29, 2016

LGTM

int hdr_len = snprintf((char*)buf, len, "$%zd\r\n", str_len);
pos_to_str_with_limit(&arg->pos, buf + hdr_len, str_len);
int postfix_len = snprintf(bytes_buf, sizeof bytes_buf, bytes_fmt, arg->pos.str_len);
memcpy(buf + max_len - 2 - postfix_len, bytes_buf, postfix_len);
Copy link

@tdsparrow tdsparrow Aug 29, 2016

Choose a reason for hiding this comment

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

max_len or len here? it took me minutes to figure out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are the same, see line 85 size_t len = min(real_len, max_len);

Choose a reason for hiding this comment

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

I figured out, it's just implicit on first sight.

@wooparadog wooparadog merged commit a25cea7 into eleme:master Aug 30, 2016
@doyoubi doyoubi mentioned this pull request Oct 31, 2016
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