feature: shdict:incr(): added the "exptime" argument to set the expiry of values when they are first created via the "init" argument.#165
Conversation
lib/resty/core/shdict.lua
Outdated
| end | ||
|
|
||
| if has_init == 0 then | ||
| error('must provide "init" when providing "exptime"', 2) |
There was a problem hiding this comment.
This is a new case of an shdict API both creating values and updating already existing ones, and now having to deal with conditionally setting their exptime as well.
Unlike set() and expire() which unconditionally replace the exptime, I made incr() only replace it when the value did not exist. If you believe this does not make sense, let me know.
lib/resty/core/shdict.lua
Outdated
| int ngx_http_lua_ffi_shdict_incr(void *zone, const unsigned char *key, | ||
| size_t key_len, double *value, char **err, int has_init, double init, | ||
| int *forcible); | ||
| int exptime, int *forcible); |
There was a problem hiding this comment.
exptime is confusing. We should call it init_ttl?
lib/resty/core/shdict.lua
Outdated
|
|
||
|
|
||
| local function shdict_incr(zone, key, value, init) | ||
| local function shdict_incr(zone, key, value, init, exptime) |
lib/resty/core/shdict.lua
Outdated
| error('bad "exptime" argument', 2) | ||
| end | ||
|
|
||
| if has_init == 0 then |
There was a problem hiding this comment.
We should use a boolean typed value here instead of using an integer? It's Lua anyway. Seems like an old issue.
There was a problem hiding this comment.
Do you want me to update this logic in the above handling of the init argument in the same commit to use a boolean instead?
There was a problem hiding this comment.
@agentzh Updated, got rid of the has_init variable altogether.
thibaultcha
force-pushed
the
feat/incr-exptime
branch
from
30794db to
0dd4072
Compare
of values when they are first created via the "init" argument.
thibaultcha
force-pushed
the
feat/incr-exptime
branch
from
0dd4072 to
60517af
Compare
|
|
||
| local rc = C.ngx_http_lua_ffi_shdict_incr(zone, key, key_len, num_value, | ||
| errmsg, has_init, init, | ||
| errmsg, init and 1 or 0, |
There was a problem hiding this comment.
OK, I now see why it used has_init as an integer in the first place...
There was a problem hiding this comment.
Yes... However, this patch still looks like an improvement to me though.
|
|
||
| if not init then | ||
| error('must provide "init" when providing "init_ttl"', 2) | ||
| end |
There was a problem hiding this comment.
I'm not really happy with such complex argument value check. It looks branchy and expensive. Will you simplify it in a future PR? Thanks!
|
Merged with comments :) Thanks! |
2 participants
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-resty-core project.
Implement the
exptimeargument inshdict:incr()as discussed in openresty/openresty#328. Contrary to #10, this is only implemented in the FFI counterpart of the shdict API.If deemed necessary (e.g., for backwards compatibility reasons), I have a commit with support for this argument in the CFunction as well. As per recent guidance, this should apparently not be necessary.
We only set/support the
exptimeargument when theinitargument is given/takes effect, as we do not wish to tamper with the expiry time of values created by other actors thanincr().Sister PR at openresty/lua-nginx-module#1226