You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/07/03 01:08:25 UTC
[GitHub] [apisix] Applenice opened a new pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Applenice opened a new pull request #4524:
URL: https://github.com/apache/apisix/pull/4524
### What this PR does / why we need it:
fix: https://github.com/apache/apisix/issues/3917
### Pre-submission checklist:
* [X] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [X] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-875208450
I think we can change the skywalking shdict back so that we can merge this one first.
It would be great if skywalking Lua client's shdict can be configured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r666928371
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
After this change, the stream module is turned on by default, which is useless in major cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r666970233
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
Indeed, if the stream function is not used, it is redundant. Then revert to this?
```
stream_proxy: # TCP/UDP proxy
tcp: # TCP proxy address list
- 9100
- "127.0.0.1:9101"
udp: # UDP proxy address list
- 9200
- "127.0.0.1:9211"
lua_shared_dict:
lrucache_lock_stream: 10m
```
Is there a good way to enable it by default in stream_proxy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874382709
@Yiyiyimu Thank you for your help, before fe8eaf7 can you help to check the reason of failure?
I don't know if it's feasible to require upstream dependencies to change, but at this point, it seems like a uniform style might not be the right option.😥
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r665827451
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
Because it said before that the default is on, so the stream will be loaded by default, and when the user turns on the stream_proxy, then turn on the relevant content
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r663626184
##########
File path: conf/config-default.yaml
##########
@@ -104,6 +104,8 @@ apisix:
# udp: # UDP proxy port list
# - 9200
# - "127.0.0.1:9201"
+ # lua_shared_dict:
+ # lrucache_lock_stream: 10m
Review comment:
okay, I'll try
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r665796523
##########
File path: conf/config-default.yaml
##########
@@ -96,6 +96,9 @@ apisix:
# see https://github.com/api7/lua-resty-radixtree/#parameters-in-path for
# more details.
ssl: 'radixtree_sni' # radixtree_sni: match route by SNI(base on radixtree)
+ stream:
+ lua_shared_dict:
+ lrucache-lock-stream: 10m
Review comment:
We can omit the stream suffix?
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
Why make this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4524:
URL: https://github.com/apache/apisix/pull/4524
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r666928371
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
After this change, the stream module is turned on by default, which is not useless in major cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r665826913
##########
File path: conf/config-default.yaml
##########
@@ -96,6 +96,9 @@ apisix:
# see https://github.com/api7/lua-resty-radixtree/#parameters-in-path for
# more details.
ssl: 'radixtree_sni' # radixtree_sni: match route by SNI(base on radixtree)
+ stream:
+ lua_shared_dict:
+ lrucache-lock-stream: 10m
Review comment:
Didn't we say before that we should enable it by default?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874968965
Hi @Applenice as you see, the failure before that commit does not occur in the current stage, so don't worry about it~
And don't hesitate to directly ask if the upstream repo accept the change, since we are in the open-source community and the best part of it is everything is public and everyone is equal! No need to question yourself before you really did it~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r667907197
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
OK, then I will change it back
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-875231614
@Yiyiyimu @spacewander Thanks. I'll modify the code later and try to give skywalking-nginx-lua a PR afterwards to see if they accept the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874140002
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu edited a comment on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Yiyiyimu edited a comment on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874968965
Hi @Applenice as you see, the failure before that commit does not occur in the current stage, so don't worry about it~
And don't hesitate to directly ask if the upstream repo accept the change, since we are in the open-source community and the best part of it is everything is public and everyone is equal! No need to question yourself before you really did it~ 😁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r667285829
##########
File path: conf/config-default.yaml
##########
@@ -96,6 +96,9 @@ apisix:
# see https://github.com/api7/lua-resty-radixtree/#parameters-in-path for
# more details.
ssl: 'radixtree_sni' # radixtree_sni: match route by SNI(base on radixtree)
+ stream:
Review comment:
I think this section should be added as the same level of the http.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874263787
Hi @Applenice I think the reason is the lib [`skywalking-nginx-lua`](https://github.com/apache/skywalking-nginx-lua) use `tracing_buffer` as the shared-dict name. Maybe you could propose a PR there to make it configurable.
I'm not sure if there are other choices, so waiting for @spacewander's opinion
```
$ grep -r 'tracing_buffer' apisix
./apisix/cli/ngx_tpl.lua: lua_shared_dict tracing_buffer 10m; # plugin: skywalking
./apisix/plugins/skywalking.lua: local metadata_shdict = ngx.shared.tracing_buffer
./conf/nginx.conf: lua_shared_dict tracing_buffer 10m; # plugin: skywalking
./t/servroot/conf/nginx.conf: lua_shared_dict tracing_buffer 10m; # plugin skywalking
./t/APISIX.pm: lua_shared_dict tracing_buffer 10m; # plugin skywalking
./deps/lib/luarocks/rocks-5.1/skywalking-nginx-lua/0.4-1/doc/README.md: lua_shared_dict tracing_buffer 100m;
./deps/lib/luarocks/rocks-5.1/skywalking-nginx-lua/0.4-1/doc/README.md: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local queue = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/tracer.lua:local metadata_shdict = ngx.shared.tracing_buffer
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r667564090
##########
File path: apisix/cli/ngx_tpl.lua
##########
@@ -57,7 +57,7 @@ env {*name*};
{% end %}
{% end %}
-{% if stream_proxy then %}
+{% if stream then %}
Review comment:
Please revert this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874140002
@spacewander Can you help me to see why the CI failed? I didn't find the reason.☹
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874968965
Hi @Applenice as you see, the failure before that commit does not occur in the current stage, so don't worry about it~
And don't hesitate to directly ask if the upstream repo accept the change, since we are in the open-source community and the best part of it is everything is public and everyone is equal. No need to question yourself before you really did it~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r663589993
##########
File path: conf/config-default.yaml
##########
@@ -104,6 +104,8 @@ apisix:
# udp: # UDP proxy port list
# - 9200
# - "127.0.0.1:9201"
+ # lua_shared_dict:
+ # lrucache_lock_stream: 10m
Review comment:
Better to create a new section:
```
stream:
lua_shared_dict:
lrucache_lock: 10m
```
So it can be enabled by default. Also, we can add the stream suffix by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#issuecomment-874263787
Hi @Applenice I think the reason is the lib [`skywalking-nginx-lua`](https://github.com/apache/skywalking-nginx-lua) use `tracing_buffer` as the shared-dict name. Maybe you could propose a PR there to make it configurable.
I'm not sure if there are other choices, so waiting for @spacewander's opinion
```
$ grep -r 'tracing_buffer' apisix
./apisix/cli/ngx_tpl.lua: lua_shared_dict tracing_buffer 10m; # plugin: skywalking
./apisix/plugins/skywalking.lua: local metadata_shdict = ngx.shared.tracing_buffer
./conf/nginx.conf: lua_shared_dict tracing_buffer 10m; # plugin: skywalking
./t/servroot/conf/nginx.conf: lua_shared_dict tracing_buffer 10m; # plugin skywalking
./t/APISIX.pm: lua_shared_dict tracing_buffer 10m; # plugin skywalking
./deps/lib/luarocks/rocks-5.1/skywalking-nginx-lua/0.4-1/doc/README.md: lua_shared_dict tracing_buffer 100m;
./deps/lib/luarocks/rocks-5.1/skywalking-nginx-lua/0.4-1/doc/README.md: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local metadata_buffer = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/client.lua: local queue = ngx.shared.tracing_buffer
./deps/share/lua/5.1/skywalking/tracer.lua:local metadata_shdict = ngx.shared.tracing_buffer
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Applenice commented on a change in pull request #4524: feat: support lua_shared_dict variable in ngx_tpl.lua loaded via configuration file
Posted by GitBox <gi...@apache.org>.
Applenice commented on a change in pull request #4524:
URL: https://github.com/apache/apisix/pull/4524#discussion_r667320375
##########
File path: conf/config-default.yaml
##########
@@ -96,6 +96,9 @@ apisix:
# see https://github.com/api7/lua-resty-radixtree/#parameters-in-path for
# more details.
ssl: 'radixtree_sni' # radixtree_sni: match route by SNI(base on radixtree)
+ stream:
Review comment:
By level, it should now be level 2, under `apisix`:
```yaml
apisix:
stream:
lua_shared_dict:
lrucache-lock-stream: 10m
```
`http` is now also at level 2, under `nginx_config`:
```yaml
nginx_config: # config for render the template to generate nginx.conf
http:
enable_access_log: true # enable access log or not, default true
```
So I need to adjust the location of the `stream` under `nginx_config`, right?😉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org