ngx.shared.DICT.incr optional expire argument #358

splitice · 2014-04-05T10:17:05Z

As mentioned in https://groups.google.com/forum/#!topic/openresty-en/-avsa-jaSYg

splitice · 2015-03-31T14:25:19Z

Can I bump this? Its an extremely simple patch for a feature I continue to find to be very useful. Primarily we use this in systems with timed states (e.g bans) based on keys stored into shared memory.

Merges cleanly into latest master.

agentzh · 2015-03-31T19:27:58Z

src/ngx_http_lua_shdict.c

-    if (n != 3) {
-        return luaL_error(L, "expecting 3 arguments, but only seen %d", n);
+    if (n < 3) {
+        return luaL_error(L, "expecting atleast 3 arguments, but only seen %d", n);


Typo: atleast => at least

agentzh · 2015-03-31T19:31:50Z

@splitice Sorry for the delay on my side. Feel free to bump tickets lacking my replies :)

In addition to the issues commented above, will you please add some unit test cases for this new feature in the existing test suite? Or even better, some some simple docs in the file doc/HttpLuaModule.wiki. Thanks!

splitice · 2015-04-01T15:01:22Z

Thank you for the feedback, I have updated the pull request to include the recommendations given (with the exception of a unit test).

I am not confident in my understanding of the unit testing system used, nor do I currently have the testing environment configured. It should not be difficult to add as the semantics are the same as with other methods including the parameter.

agentzh · 2015-04-01T19:21:03Z

src/ngx_http_lua_shdict.c

+        return luaL_error(L, "expecting at least 3 arguments, but only seen %d", n);
+    }
+
+	if (n > 4) {


Style: indentation not aligned. Maybe you're using a tab here?

Also, I think we can merge these two tests into a single test (see other functions).

agentzh · 2015-04-01T19:25:13Z

@splitice The documentation for the test scaffold can be found here:

https://github.com/openresty/lua-nginx-module#test-suite

https://github.com/openresty/test-nginx#readme

And I usually use the util/build2.sh script in the ngx_lua repos to build the nginx used by the test scaffold. Let me know if you have any specific questions regarding the test suite :) Thanks!

splitice · 2015-04-02T03:05:06Z

Fixed the stray tab.

Added better parameter handling, now accepts nil and does not silently handle values <0

Conflicts: src/ngx_http_lua_shdict.c

…n lua

splitice · 2016-01-04T04:28:36Z

Related #586

Shdict cas

# Conflicts: # doc/HttpLuaModule.wiki # src/ngx_http_lua_shdict.c

agentzh · 2016-12-16T19:48:53Z

I wonder if we can just add an expire method here? @doujiang24 already has a patch to propose I think?

doujiang24 · 2016-12-17T02:10:02Z

yeah, I think expire method is more proper, the patch will in the independent lua-shdict-nginx-module, it's coming soon.

splitice · 2016-12-17T02:15:10Z

I still feel that the incr method should match the syntax of set and add which both have an expire argument. And at-least in our use-cases for the shared dictionary (primarily as a keyed hit counter, or cache) we use expires for near every call.

Although its going to be difficult to get consistency now that the init parameter has been introduced :(

This PR has conflicts and has got messy (since it was based off our master years ago) at the moment, I'll resolve them if this ever looks likely to merge.

agentzh · 2016-12-18T04:10:19Z

@doujiang24 Separate nginx C module? I don't think we are there yet :) Better be in the lua-nginx-module core now. We can split the subsystems systematically later.

agentzh · 2016-12-18T04:12:08Z

@splitice set and add both insert new keys most of the times (if not all the times) while incr usually works on existing keys. So it's not really common to specify an expiration time on incr. To me, the expire() method is a more natural fit since it is supposed to work on existing keys.

splitice · 2016-12-18T09:10:24Z

Sorry, i'm not sure I really see your point regarding existing keys.

At-least in our usages of incr over multiple subsystems I can count on one hand the number of times we have used incr without an expire argument. Large amounts of the time its used as a key'ed counter for certain events, or in reverse to limit certain events.

In order to do this with two methods expire would have to return false if the key was not found, and then incr called with init, then if created call expire again. Three method calls (worst-case) for something we currently do with one parameter (and one rbtree lookup).

I have no objections for an exipre method as well. its definitely a plus. Theres a few places we do set variables just to touch the expire time.

agentzh · 2016-12-18T19:20:14Z

@splitice Your PR's diff contains unrelated changes like cas, which is rather confusing. Regarding the expire argument of incr(), how exactly do you define its semantics? I don't see why you need 3 method calls for your use case. Seems like two should be sufficient.

splitice · 2016-12-19T00:32:41Z

Yes, as stated this would require work before merging now. I never expected it to sit around for 2-3 years. I'll make a feature branch and re-order the parameters (we are using key, value, expire, init since for us expire came first, and its more consistent with set & add)

Heres what we currently use almost everywhere:

ngx.shared.test:incr("key_test", 1, 1, 300) -- parameter order key, value, init, expire

Heres what it would become:

local existing_key = ngx.shared.test:expire("key_test", 300) -- bump expire for 5 minutes
ngx.shared.test:incr("key_test", 1, 1) -- increment, or init if new
if not existing_key then
    ngx.shared.test:expire("key_test", 300) -- if is new, we still need to set the expire
end

agentzh · 2016-12-19T04:31:39Z

@splitice Well, we could add an optional exptime argument to incr() to avoid the small race condition between the incr() and expire() call sequence.

splitice · 2016-12-19T04:37:50Z

This Pull request adds an expires argument to incr to do just that. 3 method calls and a conditional -> 1 method call.

agentzh · 2016-12-19T05:06:33Z

@splitice Just a reminder: to make your PR merge-able, you'll have to

prepare the corresponding test cases for your new feature or changes.
update both the old CFunction-based implementation in ngx_lua's core and the FFI-based implementation in lua-resty-core.

The last time I checked, this PR lacks test cases at least.

agentzh · 2016-12-19T05:07:19Z

@splitice Thanks for your contribution!

splitice · 2016-12-19T23:26:05Z

All APIs (FFI & CFunction) have been built and work (we have used them for years), they just need to be rebased to a newer version and their arguments order switched for backwards compatibility. There is a corresponding lua-resty-core patch.

If this will get merged I'll write a simple set tests, Something like:

=== TEST N: incr expire test new key
--- http_config
    lua_shared_dict dogs 1m;
--- config
    location = /test {
        content_by_lua '
            local dogs = ngx.shared.dogs
            dogs:incr("foo", 32, 1, 0.5)
            ngx.location.capture("/sleep/0.01")
            ngx.say(dogs:get("foo"))
        ';
    }
    location ~ '^/sleep/(.+)' {
        echo_sleep $1;
    }
--- request
GET /test
--- response_body
32
--- no_error_log
[error]


=== TEST N: incr expire test new key expire
--- http_config
    lua_shared_dict dogs 1m;
--- config
    location = /test {
        content_by_lua '
            local dogs = ngx.shared.dogs
            dogs:incr("foo", 32, 1,  0.001)
            ngx.location.capture("/sleep/ 0.002")
            ngx.say(dogs:get("foo"))
        ';
    }
    location ~ '^/sleep/(.+)' {
        echo_sleep $1;
    }
--- request
GET /test
--- response_body
nil
--- no_error_log
[error]

=== TEST N: incr expire test new key expire
--- http_config
    lua_shared_dict dogs 1m;
--- config
    location = /test {
        content_by_lua '
            local dogs = ngx.shared.dogs
            dogs:incr("foo", 32, 1)
            dogs:incr("foo", 32, nil,  0.001)
            ngx.location.capture("/sleep/ 0.002")
            ngx.say(dogs:get("foo"))
        ';
    }
    location ~ '^/sleep/(.+)' {
        echo_sleep $1;
    }
--- request
GET /test
--- response_body
nil
--- no_error_log
[error]


=== TEST N: incr expire set no init
--- http_config
    lua_shared_dict dogs 1m;
--- config
    location = /test {
        content_by_lua '
            local dogs = ngx.shared.dogs
            dogs:incr("foo", 32, nil, 0.5)
            ngx.location.capture("/sleep/0.01")
            ngx.say(dogs:get("foo"))
        ';
    }
    location ~ '^/sleep/(.+)' {
        echo_sleep $1;
    }
--- request
GET /test
--- response_body
nil
--- no_error_log
[error]

If this is sufficient, I'll do the work. Otherwise if there is still debate over the feature, I'll leave it.

agentzh · 2016-12-20T01:15:10Z

@splitice The API design looks good to me from your test draft. @doujiang24 @membphis @moonbingbing ?

BTW, newly added tests should use *_by_lua_block instead of *_by_lua. Also, you'd better use ngx.sleep instead of ngx.location.capture for sleeping.

doujiang24 · 2016-12-20T06:34:33Z

LGME:)

moonbingbing · 2016-12-20T08:15:24Z

LGTM

membphis · 2016-12-21T23:40:11Z

LGME :)

splitice added 2 commits April 5, 2014 21:13

ngx.shared.DICT.incr optional expire argument

a1df068

tabs fixed, replaced with spaces

a315d79

agentzh force-pushed the master branch from 701dd52 to cd01e7b Compare December 8, 2014 05:23

Merge branch 'upstream/master'

8b5d786

agentzh reviewed Mar 31, 2015
View reviewed changes

splitice added 2 commits April 2, 2015 01:56

Update code to meet nginx coding standards

c11df99

Documentation updated to include incr's exptime parameter

b4c12d6

agentzh reviewed Apr 1, 2015
View reviewed changes

splitice added 2 commits April 2, 2015 13:54

Fixed stray tab

7626688

nil handling

3caef80

splitice added 9 commits April 8, 2015 22:41

Merge branch 'openresty/master'

c84f1c9

Merge branch 'openresty/master'

248afcf

Merge remote-tracking branch 'openresty/master'

aaf49e6

Conflicts: src/ngx_http_lua_shdict.c

fix commited conflict

e4f79de

Merge remote-tracking branch 'refs/remotes/openresty/master'

8ecf938

Merge remote-tracking branch 'refs/remotes/openresty/master'

ba156b9

add exptime argument to ffi method with the same semantics as :incr i…

0fea525

…n lua

Merge remote-tracking branch 'refs/remotes/openresty/master'

16427d0

add tp local variable was undefined

4e618cf

splitice and others added 9 commits May 11, 2016 22:07

Merge pull request #1 from doujiang24/shdict_cas

eb88b62

Shdict cas

Merge remote-tracking branch 'refs/remotes/openresty/master'

ddeea25

add user_flags to incr return value

efd0717

user flags not defined

bae63ad

Merge remote-tracking branch 'refs/remotes/openresty/master'

83be5d4

add see other to ngx.redirect

ebd0d57

fix previous commit

4cc4a08

Merge remote-tracking branch 'refs/remotes/openresty/master'

c37cedc

Merge remote-tracking branch 'refs/remotes/openresty/master'

d47fb20

# Conflicts: # doc/HttpLuaModule.wiki # src/ngx_http_lua_shdict.c

agentzh force-pushed the master branch from ab60f70 to 397f366 Compare October 31, 2016 23:56

fix incr init bug

e3b2c09

splitice mentioned this pull request Dec 29, 2016

ngx.shared.DICT.incr optional expire argument #942

Closed

splitice closed this Dec 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngx.shared.DICT.incr optional expire argument #358

ngx.shared.DICT.incr optional expire argument #358

splitice commented Apr 5, 2014

splitice commented Mar 31, 2015

agentzh Mar 31, 2015

agentzh commented Mar 31, 2015

splitice commented Apr 1, 2015

agentzh Apr 1, 2015

agentzh commented Apr 1, 2015

splitice commented Apr 2, 2015

splitice commented Jan 4, 2016

agentzh commented Dec 16, 2016

doujiang24 commented Dec 17, 2016

splitice commented Dec 17, 2016

agentzh commented Dec 18, 2016

agentzh commented Dec 18, 2016

splitice commented Dec 18, 2016 •

edited

Loading

agentzh commented Dec 18, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 19, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 19, 2016 •

edited

Loading

agentzh commented Dec 19, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 20, 2016

doujiang24 commented Dec 20, 2016

moonbingbing commented Dec 20, 2016

membphis commented Dec 21, 2016

ngx.shared.DICT.incr optional expire argument #358

ngx.shared.DICT.incr optional expire argument #358

Conversation

splitice commented Apr 5, 2014

splitice commented Mar 31, 2015

agentzh Mar 31, 2015

Choose a reason for hiding this comment

agentzh commented Mar 31, 2015

splitice commented Apr 1, 2015

agentzh Apr 1, 2015

Choose a reason for hiding this comment

agentzh commented Apr 1, 2015

splitice commented Apr 2, 2015

splitice commented Jan 4, 2016

agentzh commented Dec 16, 2016

doujiang24 commented Dec 17, 2016

splitice commented Dec 17, 2016

agentzh commented Dec 18, 2016

agentzh commented Dec 18, 2016

splitice commented Dec 18, 2016 • edited Loading

agentzh commented Dec 18, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 19, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 19, 2016 • edited Loading

agentzh commented Dec 19, 2016

splitice commented Dec 19, 2016

agentzh commented Dec 20, 2016

doujiang24 commented Dec 20, 2016

moonbingbing commented Dec 20, 2016

membphis commented Dec 21, 2016

splitice commented Dec 18, 2016 •

edited

Loading

agentzh commented Dec 19, 2016 •

edited

Loading