bzp2010 commented on code in PR #12200:
URL: https://github.com/apache/apisix/pull/12200#discussion_r2096844497
##########
apisix/cli/config.lua:
##########
@@ -112,6 +112,7 @@ local _M = {
["worker-events-stream"] = "10m",
["tars-stream"] = "1m",
["upstream-healthcheck-stream"] = "10m",
+ ["status-report-stream"] = "1m",
Review Comment:
Is it necessary to have a special shdict for stream?
We can declare a shared memory block under the http and stream subsystems
with
```
meta {
lua_shared_dict x 1m;
}
```
Thus the two memory blocks defined in http and stream, respectively, can be
combined into one.
##########
apisix/core/config_yaml.lua:
##########
@@ -45,11 +45,16 @@ local pcall = pcall
local io = io
local ngx = ngx
local re_find = ngx.re.find
+local process = require("ngx.process")
+local worker_id = ngx.worker.id
local apisix_yaml_path = profile:yaml_path("apisix")
local created_obj = {}
local shared_dict
-
-
+local status_report_shared_dict_name = "status-report"
+local is_http = ngx.config.subsystem == "http"
+if not is_http then
+ status_report_shared_dict_name = status_report_shared_dict_name ..
"-stream"
+end
Review Comment:
We don't need this separate branch anymore do we?
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -550,6 +558,23 @@ http {
}
{% end %}
+ {% if status then %}
+ server {
+ listen {* status_server_addr *} enable_process=privileged_agent;
+ access_log off;
+ location /status {
+ content_by_lua_block {
+ apisix.status()
+ }
+ }
+ location /status/ready {
+ content_by_lua_block {
+ apisix.status_ready()
+ }
+ }
+ }
Review Comment:
Depending on your implementation (and the as is situation with NGINX), this
server block will always be defined under the http subsystem, so when the user
tries to access `/status/ready`, the request is handled by
`apisix.status_ready()`.
You use
https://github.com/apache/apisix/pull/12200/files#diff-7439ffe94cc4bcad12066d5d83f6c1004b8e9b3ff5097b645fa7c700eb3b3743R886
with
```
local status_shdict = ngx.shared["status-report"] or
ngx.shared["status-report-stream"]
```
to get the desired shdict, and since the request is handled in the http
subsystem, it will always get the `status-report` instead of the
`status-report-stream`, so the stream health check you implemented is
meaningless.
Now, adding http lua_shared_dict to nginx.conf is conditional on
```
http.lua_shared_dict["status-report"]
```
being present, which means that, by default, it he always present, so
there's no chance of you getting `status-report-stream`.
---
Considering it from another side, both config_etcd and config_yaml will
register some timers in `init_worker` to load the full configuration from the
configuration provider (both route and stream route included).
This always happens in http and stream (i.e. the loaded configuration for
http also contains the stream route, and vice versa), and since the user always
has at least one subsystem enabled (either http or stream), the status of the
worker's health can be reported by either subsystem, and reporting at the same
time is probably fine.
Also, although http and stream are different subsystems, they both run in
the same worker process, so when the http route configuration is loaded
successfully, it also means that the stream route is loaded successfully, and a
successful configuration load means that the worker is healthy.
Therefore, using a shared shdict would be feasible and necessary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
