Skip to content

feature: add api in shdict: lpush, lpop, rpush, rpop, llen #586

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

Closed
wants to merge 11 commits into from

Conversation

doujiang24
Copy link
Member

I'm not sure if it is an good idea to get the list queue head in the way:

#define ngx_http_lua_shdict_get_list_head(sd, key_len)                      \
    (ngx_queue_t *) ngx_align_ptr(((u_char *) &sd->data + key_len),         \
                                  NGX_ALIGNMENT)

Anything not suitable, let me know, thanks :)

}



Copy link
Member

Choose a reason for hiding this comment

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

Style: 2 blank lines needed instead of 3.

@agentzh
Copy link
Member

agentzh commented Oct 28, 2015

@doujiang24 The feature implemented in this PR is awesome. I'll have a closer look as soon as I can manage (need to cut a new OpenResty release ASAP).

@bungle
Copy link
Member

bungle commented Oct 28, 2015

Yes, this is great, thanks @doujiang24. I was going to look at implementing just one thing, as discussed in #552, but you went ahead and implemented a lot more of these. Maybe when you are up to the speed with this, you can also take a look at implementing ttl ( expire's cousin, :-) )?

@doujiang24
Copy link
Member Author

@agentzh Thanks for your review :)
I just fixed some code style and a stupid bug.

@doujiang24
Copy link
Member Author

@bungle Thanks, I tried to implement ttl and type api, but it brought some duplicated codes. I haven't come up with an idea to fix it. I'll try again later after this PR is merged :)

@thibaultcha
Copy link
Member

Wow, that is really nice! Good job.

@dualface
Copy link

dualface commented Jan 1, 2016

+1

@guanglinlv
Copy link
Contributor

++1

@agentzh agentzh force-pushed the master branch 2 times, most recently from e7ac10c to cfd4f90 Compare January 31, 2016 19:04
@agentzh agentzh mentioned this pull request May 18, 2016


enum {
SHDICT_USERDATA_INDEX = 1,
};


#define ngx_http_lua_shdict_get_list_head(sd, key_len) \
Copy link
Member

Choose a reason for hiding this comment

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

Shall we use an inline function here instead of a macro? Thanks!

@agentzh
Copy link
Member

agentzh commented Jul 30, 2016

@doujiang24 Will you please rebase it to the latest master branch? Thanks!


lua_pushnumber(L, (lua_Number) sd->value_len);

ngx_shmtx_unlock(&ctx->shpool->mutex);
Copy link
Member

Choose a reason for hiding this comment

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

Better put ngx_shmtx_unlock() before lua_pushnumber() because the latter might throw. Thanks!


p = ngx_copy(sd->data, key, key_len);
ngx_memcpy(p, str_value_buf, str_value_len);
} else if (rc == NGX_OK) {
Copy link
Member

Choose a reason for hiding this comment

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

�I don't think we need an else here since the previous branch always jumps away via goto push_node, right?

@agentzh
Copy link
Member

agentzh commented Jul 30, 2016

It seems to me that the expire method is irrelevant to the rest of the changes in this PR. I think it's better to move it to a separate PR to simplify the review and merge work. What do you think? I prefer small and self-contained PRs over mixed ones.

@doujiang24
Copy link
Member Author

@agentzh Thanks for your review :)
All fixed and the expire method may be another PR after this PR merged :)

@agentzh agentzh changed the title feature: add api in shdict: lpush, lpop, rpush, rpop, llen, expire feature: add api in shdict: lpush, lpop, rpush, rpop, llen Aug 2, 2016
@@ -1534,7 +2206,7 @@ ngx_http_lua_find_zone(u_char *name_data, size_t name_len)
if (name->len == name_len
&& ngx_strncmp(name->data, name_data, name_len) == 0)
{
return &zone[i];
return zone;
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this change is wrong and bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this may be something wrong from rebase :(
I have rechecked the left change :)

@agentzh
Copy link
Member

agentzh commented Aug 8, 2016

Merged into master already.

@agentzh agentzh closed this Aug 8, 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.

7 participants