[go: up one dir, main page]

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

Add support for SELECT command #115

Merged
merged 3 commits into from
Mar 17, 2017
Merged

Add support for SELECT command #115

merged 3 commits into from
Mar 17, 2017

Conversation

tevino
Copy link
Contributor
@tevino tevino commented Mar 14, 2017

No description provided.

@tevino tevino requested a review from maralla March 14, 2017 11:28
@tevino tevino mentioned this pull request Mar 14, 2017
@tremez
Copy link
tremez commented Mar 14, 2017

Thank you !
Waiting, when it will appear in master

src/command.c Outdated
struct pos_array *db_str = &db_data->pos;
// TODO: Implement pos_to_long then use it here, someday.
if (db_str->str_len == 1 && db_str->items->str[0] == '0') {
conn_add_data(cmd->client, (uint8_t*)rep_ok, 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the overhead of str_len is acceptable.

src/command.c Outdated

struct pos_array *db_str = &db_data->pos;
// TODO: Implement pos_to_long then use it here, someday.
if (db_str->str_len == 1 && db_str->items->str[0] == '0') {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comparison is not very suitable here. If the command select 000 is issued, OK should be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, are you suggesting pos_to_long?

ASSERT_TYPE(data, REP_ARRAY);
ASSERT_ELEMENTS(data->elements == 2, data);

struct redis_data *db_data = &data->element[1];
Copy link
Contributor
@doyoubi doyoubi Mar 16, 2017

Choose a reason for hiding this comment

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

The second argument of SELECT is the index of database. The name db_data is not quite suitable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's the index of db, I think db tells more than index here.

@@ -559,3 +559,23 @@ size_t pos_to_str_with_limit(struct pos_array *pos, uint8_t *str, size_t limit)
}
return len;
}

int pos_is_zero(struct pos_array *pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Redis uses strtoll which is available in C99 for error handling. Both of us just missed it!

Copy link
Contributor Author
@tevino tevino Mar 16, 2017

Choose a reason for hiding this comment

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

To make use of strtoll, a pos_array must be converted into a string first, too expensive for an operation like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we try our best to conform to redis, we should consider this:

> select hello
(error) ERR invalid DB index

Copy link
Contributor Author
@tevino tevino Mar 16, 2017

Choose a reason for hiding this comment

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

Agree, I've thought about that.

The error messages are completely different in some cases:

Redis:
> select
(error) ERR wrong number of arguments for 'select' command

Corvus:
> select
(error) ERR Proxy fail to forward command

I'm afraid it's too much work to get them exactly the same in this PR.

And if nothing goes wrong, do we really have to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think supporting "SELECT is not allowed in cluster mode" is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's enough.

src/parser.c Outdated
}

int len = 0;
for (size_t i = 0; len < length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

length here should be pos->pos_len. See cmd_get_map_key for how to iterate pos_array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But pos_to_str uses pos->str_len instead of pos->pos_len.

Copy link
Contributor

Choose a reason for hiding this comment

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

pos_array is used to represent only one string, consisting of split sub string stored in items

struct pos_array {
    struct pos *items;
    int str_len;                    // str_len == sum(items.len)
    int pos_len;                  // pos_len == len(items)
    int max_pos_size;       // max_pos_size is the actual memory size allocated
};

Copy link
Contributor Author
@tevino tevino Mar 16, 2017

Choose a reason for hiding this comment

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

Actually that's what I'm thinking about at the beginning, then I find this and missread .pos_len = 2 into .pos_len = 12.

Thanks for the hint, what about now?

@tevino tevino merged commit ed3ad26 into eleme:master Mar 17, 2017
@tevino tevino deleted the cmd-select branch March 17, 2017 06:58
@doyoubi doyoubi mentioned this pull request May 9, 2017
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