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

splitice · 2016-12-29T06:08:16Z

Rebased #358

I'll do a future commit changing all the shdict tests to _block.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

splitice · 2016-12-29T06:09:13Z

Let me know if its good and I'll do stream & resty-core.

membphis · 2016-12-29T09:59:08Z

src/ngx_http_lua_shdict.c

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


This line exceeds 80 columns. Please use the ngx-releng tool to check such things.

splitice · 2016-12-30T02:49:08Z

done

membphis · 2016-12-30T02:56:24Z

t/043-shdict.t

+    lua_shared_dict dogs 1m;
+--- config
+    location = /test {
+        content_by_lua '


Newly added test cases should use the *_by_lua_block form instead.

I'll do a future PR changing all the shdict tests to _block, not just these ones. It's an easy regex.

@splitice I prefer keeping the old tests intact. Just the newly added ones should use *_by_lua_block.

Feel free to make adjustments.

@splitice You want me to make adjustments in your branch? Sorry, no, I can't, obviously :)

@splitice We can only help with the review process since we have many other items with much higher priorities. Thanks for your understanding.

agentzh · 2017-01-09T19:22:04Z

src/ngx_http_lua_shdict.c

+        sd->expires = (uint64_t) tp->sec * 1000 + tp->msec
+            + (uint64_t) (exptime * 1000);
+
+    } else if (exptime == 0) {


Better make exptime < 0 turn into exptime == 0 as well. So better replace this else if with else directly?

Those are different code paths. Not adjusting the exptime (<0 aka -1 the default for null) vs setting it to not expire (0)

@splitice Okay, got it.

agentzh · 2017-01-09T19:22:49Z

src/ngx_http_lua_shdict.c

+            if (init < 0) {
+                return luaL_error(L, "bad \"init\" argument");
+            }
+            has_init = 1;


Style: need a blank line after the } line.

agentzh · 2017-01-09T19:46:07Z

@splitice Have you created the corresponding PR for lua-resty-core's FFI-based API?

agentzh · 2017-01-09T19:47:39Z

@splitice Thank you for your contribution! Will you please look at my comments and questions above? Thanks!

agentzh · 2017-01-09T19:48:26Z

I think we also need updates in the documentation :)

agentzh · 2017-01-09T19:53:04Z

Hmm, seems like we should permanently switch over to the FFI-based implementations to lua-resty-core. I'm already sick of maintaining two parallel implementations of the same Lua API :) I bet you can feel the pain as well :)

@doujiang24 @membphis @moonbingbing @thibaultcha @yangshuxin Shall we? ;)

thibaultcha · 2017-01-09T20:05:32Z

mm, seems like we should permanently switch over to the FFI-based implementations to lua-resty-core.

I think so too. Having to provide two implementations for each new feature can also discourage a lot of contributors (I think it has been the case a few times already) and we may be missing a few opportunities to add relevant features.

However, this does raise a few concerns on the long term:

if more and more new features of the API are only provided in lua-resty-core, is it still user-friendly to ask of our users to manually require resty.core or the specific modules they need in every project they maintain?
the same way, library authors might use some future functions that are only implemented in the lua-resty-core module, and thus, using such libraries would load lua-resty-core without the user being aware of it. A cleaner way is probably for the library to require that users load resty.core in their init phase. This relates to the first point in that it is not as user-friendly.

agentzh · 2017-01-09T20:09:53Z

@thibaultcha To answer your concerns:

We automatically require resty.core before running init_by_lua* unless lua_use_resty_core off is configured in the user nginx.conf (which is on by default).
It's not a problem IMHO, since ngx_lua will require lua-resty-core and lua-resty-lrucache unless lua_use_resty_core is explicitly turned off :)

agentzh · 2017-01-09T20:11:27Z

Another upside of this is that the ngx_lua core will also be smaller :) The downside is that we now require lua-resty-core and lua-resty-lrucache for ngx_lua. The latter is not a problem if the user uses OpenResty, of course.

thibaultcha · 2017-01-09T20:12:28Z

We automatically require resty.core before running init_by_lua* unless lua_use_resty_core off

Is this an existing feature or a suggestion to solve those concerns? If so, I was about to suggest the same behavior as well (and disable it by default when not using LuaJIT).

agentzh · 2017-01-09T20:27:41Z

@thibaultcha It's a suggestion. I think I'd also drop the support for the standard Lua 5.1 interpreter in ngx_lua and openresty. LuaJIT is already diverged a lot from it and it's becoming a big burden for us due to the lack of FFI in the standard Lua 5.1 interpreter, for example.

In addition, the user can selectively choose which resty.core.* submodules to load to reduce memory footprint and GC overhead even further, as in

lua_use_resty_core regex shdict;  # only load the modules resty.core.regex and resty.core.shdict

By default, it could be lua_use_resty_core all. And the user can configure lua_use_resty_core none to disable everything :)

agentzh · 2017-01-09T20:29:25Z

@thibaultcha Alas. We should have created a separate ticket for it.

agentzh · 2017-01-09T20:39:08Z

@thibaultcha Let's move our discussion over to #949 to avoid off-topic :) Thanks!

splitice · 2017-01-09T21:56:23Z

Like this feature there has been a PR for lua-resty-core for 2 years. It needs a rebase & for the argument order to be re-ordered.

There is also a PR for the stream module that needs upgrading for the argument order same as this one.

agentzh · 2017-01-09T22:06:10Z

@splitice For your pull requests to be merged, they have to pass the review process. Thank you for your understanding :)

agentzh · 2017-01-09T22:10:23Z

@splitice So please do not blame us for not merging your old PRs for years. They just never passed the review process like the lack of test cases and etc in your old PRs.

agentzh · 2017-01-09T22:11:50Z

@splitice I do appreciate your contributions a lot and I also hope that you can respect our review process and PR merging policies. Thank you!

agentzh · 2017-01-09T22:13:37Z

@splitice We're already trying our best to collaborate and cooperate with our contributors. I hope that you can see that from our active review feedback and etc.

agentzh · 2017-01-09T22:15:37Z

@splitice Consider that these are all volunteering work.

splitice · 2017-01-09T23:46:59Z

And I too am volunteering my time to merge upstream suitable features developed on projects I am involved with. We are all volunteers here. It's fine to do a review but the constant back and forth over small style fixes that would have taken you less time to fix than to comment on is just wasteful. Due to the time wasted by this not even being looked at that I am now investing time in rebasing & re-ordering the parameters (the new order is only required due to the time-lag, and IMHO less consistent ordering considering set/add).

At the end of the day its your project and you can accept changes however you please. But just take a look at the ancient PRs, plenty of good features abandoned there. Plenty that even look complete.

As this was submitted 2-3 years ago and never processed, none of our other patches have been even considered for pushing upstream. To give you an idea of the PRs that you have missed out on from this project alone:

ngx.redirect is missing support for HTTP See Other
user_flags in incr's return value consistent with get (quite useful for setting targets and some atomic structures)

Now back to this PR, you can quite easily make any minor style changes you so desire. You should know how, merge it into a feature branch, add a commit and then merge to master. Done and dusted. Going back and forth with minor style changes over years is only wasting both of our times. I'm sure it takes you longer to write your comment than to add a newline.

Now I could go change all the newly added tests, I could also upgrade the existing with the same regex as a separate PR but realistically it's a waste of time as it looks like this code will be replaced with resty-core soon so not worth the time investment. It will either require a re-base, or removed depending on how the code ends up structured. Either way.

Continuing to consider this a fork is fine for us at this point as the path of least resistance.

thibaultcha · 2019-08-08T22:48:47Z

This function does not exist anymore since we removed the CFunction implementation (as originally discussed in this PR!) in 947fa00. The FFI implementation of this API has support for the init_ttl argument. Therefore, I will be closing this PR. Thanks for the patch!

splitice · 2019-08-08T23:07:42Z

FYI init_ttl != expire for anyone using my fork (which I continue to maintain). init_ttl does what it name says while expire continues to bump the TTL (i.e for LRU pattern)

thibaultcha · 2019-08-09T01:52:54Z

@splitice Good to know (and important distinction), thanks for clarifying.

splitice added 2 commits December 29, 2016 16:27

incr expire adrgument

a4640ce

fix init path

7c4514f

membphis reviewed Dec 29, 2016

View reviewed changes

splitice added 3 commits December 30, 2016 12:36

fix > 80 lines

2e760ec

style

3aa45d4

style

6207e36

membphis reviewed Dec 30, 2016

View reviewed changes

agentzh reviewed Jan 9, 2017

View reviewed changes

agentzh requested changes Jan 9, 2017

View reviewed changes

agentzh mentioned this pull request Jan 9, 2017

Switch over to lua-resty-core and retire old CFunction-based Lua API impl #949

Open

Style: add newline

285c498

p0pr0ck5 mentioned this pull request May 30, 2017

feat(clustering) retiring Serf - new cache invalidation mechanism Kong/kong#2561

Merged

4 tasks

p0pr0ck5 mentioned this pull request Jan 8, 2018

Rate Limiting plugin memory leak issue Kong/kong#3124

Closed

thibaultcha closed this Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

splitice commented Dec 29, 2016

splitice commented Dec 29, 2016

membphis Dec 29, 2016

splitice commented Dec 30, 2016

membphis Dec 30, 2016

splitice Dec 30, 2016

agentzh Jan 9, 2017

splitice Jan 9, 2017

agentzh Jan 9, 2017

agentzh Jan 9, 2017 •

edited

Loading

agentzh Jan 9, 2017

splitice Jan 9, 2017

agentzh Jan 9, 2017

agentzh Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017 •

edited

Loading

thibaultcha commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017 •

edited

Loading

thibaultcha commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

splitice commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

splitice commented Jan 9, 2017 •

edited

Loading

thibaultcha commented Aug 8, 2019

splitice commented Aug 8, 2019

thibaultcha commented Aug 9, 2019

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

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

Conversation

splitice commented Dec 29, 2016

splitice commented Dec 29, 2016

Choose a reason for hiding this comment

splitice commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017 • edited Loading

thibaultcha commented Jan 9, 2017 • edited Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017 • edited Loading

thibaultcha commented Jan 9, 2017 • edited Loading

agentzh commented Jan 9, 2017 • edited Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

splitice commented Jan 9, 2017 • edited Loading

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

agentzh commented Jan 9, 2017

splitice commented Jan 9, 2017 • edited Loading

thibaultcha commented Aug 8, 2019

splitice commented Aug 8, 2019

thibaultcha commented Aug 9, 2019

agentzh Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017 •

edited

Loading

thibaultcha commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017 •

edited

Loading

thibaultcha commented Jan 9, 2017 •

edited

Loading

agentzh commented Jan 9, 2017 •

edited

Loading

splitice commented Jan 9, 2017 •

edited

Loading

splitice commented Jan 9, 2017 •

edited

Loading