Copilot commented on code in PR #13904:
URL: https://github.com/apache/apisix/pull/13904#discussion_r3911244003
##########
apisix/admin/standalone.lua:
##########
@@ -64,19 +120,105 @@ local function update_and_broadcast_config(apisix_yaml)
if shared_dict then
-- the worker that handles Admin API calls is responsible for writing
the shared dict
- local ok, err = shared_dict:set("config", raw)
+ local stored = encode_config(apisix_yaml[METADATA_DIGEST], raw)
+ local ok, err = shared_dict:set("config", stored)
if not ok then
return nil, "failed to save config to shared dict: " .. err
end
core.log.info("standalone config updated: ", raw)
else
core.log.crit(config_yaml.ERR_NO_SHARED_DICT)
end
- return events:post(EVENT_UPDATE, EVENT_UPDATE)
+ return true
end
local validate_configuration = config_validate.validate_configuration
+
+local MAX_WAIT_MS = 60000
+local POLL_INTERVAL = 0.05
+
+
+local function parse_wait_ms(ctx)
+ local args = core.request.get_uri_args(ctx)
+ local wait = args and tonumber(args.wait)
Review Comment:
`get_uri_args` can return `true` for `?wait` and a table for repeated
parameters such as `?wait=1&wait=2`; passing either to `tonumber` raises a Lua
argument error and turns an otherwise accepted update into a 500 response.
Type-check the scalar before conversion, preserving the current zero-wait
behavior for invalid values.
##########
apisix/admin/standalone.lua:
##########
@@ -40,22 +58,60 @@ local METADATA_DIGEST = "X-Digest"
local _M = {}
+
+-- the "config" key in the standalone-config shared dict stores
+-- "<digest length>\n<digest><json body>" instead of plain JSON, so a worker
+-- can compare digests without decoding the (potentially large) JSON on
+-- every poll
+-- the reason for combining them into a single string and writing them to
+-- only one key is to ensure concurrent safety for updates by multiple
+-- workers
+local CONFIG_DIGEST_LENGTH_SEPARATOR = "\n"
+
+local function encode_config(digest, raw)
+ digest = digest or ""
+ return #digest .. CONFIG_DIGEST_LENGTH_SEPARATOR .. digest .. raw
+end
+
+
+local function decode_config(stored)
+ local idx = str_find(stored, CONFIG_DIGEST_LENGTH_SEPARATOR, 1, true)
+ if not idx then
+ return nil, nil, "missing digest length prefix"
Review Comment:
The shared dictionary survives reloads, but released versions store `config`
as plain JSON. Rejecting any value without this prefix means new workers cannot
restore the active configuration on upgrade; additionally, old graceful workers
cannot decode a prefixed value written during a mixed-version reload. Introduce
a migration/versioning strategy that remains readable in both directions rather
than replacing the format in place.
##########
apisix/admin/standalone.lua:
##########
@@ -313,6 +476,14 @@ end
function _M.init_worker()
+ local ok, err = try_restore_from_shared_dict()
Review Comment:
This restore is immediately discarded during startup. Both HTTP and stream
call `admin.init_worker()` before `core.config.init_worker()`
(`apisix/init.lua:140-147` and `1361-1364`), while YAML API mode resets
`apisix_yaml` to `{}` in `config_yaml.lua:555-557`. Consequently, the later
`core.config.new` calls still synchronously see an empty configuration, so the
empty-config reload window remains. Please preserve the restored table or
reorder initialization so restoration occurs after that reset and before
resource initialization.
##########
apisix/core/config_local.lua:
##########
@@ -68,4 +69,29 @@ function _M.local_conf(force)
end
+
+---
+-- Check whether the stream subsystem is enabled.
+-- Reads `apisix.proxy_mode` from the local config and looks for the
+-- substring "stream" in it, so it covers both `proxy_mode: stream` and
+-- `proxy_mode: http&stream`. `proxy_mode` doesn't change once a worker is
+-- running, so the result is computed once and cached.
+--
+-- @function require("core.config_local").is_stream_enabled
+-- @treturn boolean Whether the stream subsystem is enabled.
+-- @usage
+-- local config_local = require("core.config_local")
Review Comment:
The added usage example imports a module path that does not exist, so
copying it raises a module-not-found error; production call sites import
`apisix.core.config_local`. The LDoc function annotation should also follow the
existing `core.config_local.local_conf` form above rather than embedding
`require(...)`.
##########
apisix/admin/standalone.lua:
##########
@@ -64,19 +120,105 @@ local function update_and_broadcast_config(apisix_yaml)
if shared_dict then
-- the worker that handles Admin API calls is responsible for writing
the shared dict
- local ok, err = shared_dict:set("config", raw)
+ local stored = encode_config(apisix_yaml[METADATA_DIGEST], raw)
+ local ok, err = shared_dict:set("config", stored)
if not ok then
return nil, "failed to save config to shared dict: " .. err
end
core.log.info("standalone config updated: ", raw)
else
core.log.crit(config_yaml.ERR_NO_SHARED_DICT)
end
- return events:post(EVENT_UPDATE, EVENT_UPDATE)
+ return true
end
local validate_configuration = config_validate.validate_configuration
+
+local MAX_WAIT_MS = 60000
+local POLL_INTERVAL = 0.05
+
+
+local function parse_wait_ms(ctx)
+ local args = core.request.get_uri_args(ctx)
+ local wait = args and tonumber(args.wait)
+ if not wait or wait <= 0 then
+ return 0
+ end
+ if wait > MAX_WAIT_MS then
+ return MAX_WAIT_MS
+ end
+ return wait
Review Comment:
Please document the new `wait` parameter and its 200/202 response semantics
in `docs/en/latest/deployment-modes.md`. That public guide currently only shows
an unqualified PUT, and line 176 still states that workers start with an empty
configuration, which this PR intentionally changes by restoring from shared
memory.
##########
apisix/admin/standalone.lua:
##########
@@ -64,19 +120,105 @@ local function update_and_broadcast_config(apisix_yaml)
if shared_dict then
-- the worker that handles Admin API calls is responsible for writing
the shared dict
- local ok, err = shared_dict:set("config", raw)
+ local stored = encode_config(apisix_yaml[METADATA_DIGEST], raw)
+ local ok, err = shared_dict:set("config", stored)
if not ok then
return nil, "failed to save config to shared dict: " .. err
end
core.log.info("standalone config updated: ", raw)
else
core.log.crit(config_yaml.ERR_NO_SHARED_DICT)
end
- return events:post(EVENT_UPDATE, EVENT_UPDATE)
+ return true
end
local validate_configuration = config_validate.validate_configuration
+
+local MAX_WAIT_MS = 60000
+local POLL_INTERVAL = 0.05
+
+
+local function parse_wait_ms(ctx)
+ local args = core.request.get_uri_args(ctx)
+ local wait = args and tonumber(args.wait)
+ if not wait or wait <= 0 then
+ return 0
+ end
+ if wait > MAX_WAIT_MS then
+ return MAX_WAIT_MS
+ end
+ return wait
+end
+
+
+local function all_workers_applied(target_digest)
+ if not status_shared_dict then
+ return false
+ end
+
+ local n = worker_count()
+ local check_stream = config_local.is_stream_enabled()
+ for key in pairs(APPLIED_CHECK_KEYS) do
+ for id = 0, n - 1 do
+ if HTTP_RESOURCE_KEYS[key] then
Review Comment:
Only active configuration objects can publish these status keys. For
example, `/protos` is created solely by the `grpc-transcode` plugin
(`plugins/grpc-transcode/proto.lua:258-263`), so a valid deployment that
disables that plugin never publishes `worker:*:http:protos`; every request with
`wait>0` therefore times out with 202 even after all applicable configuration
is loaded. Restrict checks to resource objects instantiated in this subsystem.
--
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]
