AlinsRan commented on code in PR #13720:
URL: https://github.com/apache/apisix/pull/13720#discussion_r3718045623
##########
apisix/admin/init.lua:
##########
@@ -408,15 +435,43 @@ local function reload_plugins(data, event, source, pid)
ver = plugins_conf_ver_dict:get(PLUGINS_CONF_VERSION_KEY)
end
- plugin.load()
+ local ok, err = plugin.load()
+ -- record the sampled version even when the load failed: the reconciliation
+ -- timer would otherwise retry the very same plugin set every second, and
+ -- every attempt tears the live set down and rebuilds it. The cost is that
+ -- this worker keeps its previous set until some later reload bumps the
+ -- version again -- a load that failed for a transient reason (reading the
+ -- plugin file while it was still being written, say) does not recover on
+ -- its own, it only logs
if ver then
applied_plugins_conf_version = ver
end
+ if not ok then
+ return nil, err
+ end
+
if ngx_worker_id() == 0 then
sync_local_conf_to_etcd()
end
+
+ return true
+end
+
+
+local function reload_plugins(data, event, source, wid)
+ if wid == ngx_worker_id() then
+ -- this worker has already reloaded synchronously while serving the
+ -- Admin API request, see post_reload_plugins()
+ return
+ end
Review Comment:
Checked this against the library and then measured it — the 4th argument
here is the worker id, and switching to `pid` makes the publishing worker
reload twice rather than once. Reverted in 86eb908db.
The naming precedent is real but misleading. `apisix/events.lua` builds on
`resty.events.compat`, and that layer only restores the old *registration* API
— its `register()` hands the callback straight to the underlying `subscribe()`
without rewrapping the arguments, so the callback keeps the native signature
`(data, event, source, wid)`. In lua-resty-events, `callback.lua` passes
`d.wid` to the handler and `worker.lua` fills that field from
`ngx.worker.id()`, never from the pid. The one in-tree handler that still names
the argument `pid` (`apisix/discovery/consul_kv/init.lua:56`) only logs it as
`"server pid:"`, so the wrong name never surfaced there.
Measured with `t/cli/test_plugin_reload_transaction.sh`, which logs every
plugin `init`/`destroy` through a real gateway process:
- `wid == ngx.worker.id()` → one round per successful reload: `b destroy,a
destroy,a init,b init`
- `pid == ngx.worker.pid()` → two rounds, the second being exactly the
self-broadcast this check exists to skip
So with the suggestion applied every successful reload would tear the live
plugin set down and rebuild it twice on the serving worker.
##########
apisix/control/router.lua:
##########
@@ -199,9 +199,18 @@ end
end -- do
-local function reload_plugins()
+local function reload_plugins(data, event, source, wid)
+ if wid == ngx.worker.id() then
+ -- already reloaded synchronously in post_reload_plugins()
+ return
+ end
Review Comment:
Checked this against the library and then measured it — the 4th argument
here is the worker id, and switching to `pid` makes the publishing worker
reload twice rather than once. Reverted in 86eb908db.
The naming precedent is real but misleading. `apisix/events.lua` builds on
`resty.events.compat`, and that layer only restores the old *registration* API
— its `register()` hands the callback straight to the underlying `subscribe()`
without rewrapping the arguments, so the callback keeps the native signature
`(data, event, source, wid)`. In lua-resty-events, `callback.lua` passes
`d.wid` to the handler and `worker.lua` fills that field from
`ngx.worker.id()`, never from the pid. The one in-tree handler that still names
the argument `pid` (`apisix/discovery/consul_kv/init.lua:56`) only logs it as
`"server pid:"`, so the wrong name never surfaced there.
Measured with `t/cli/test_plugin_reload_transaction.sh`, which logs every
plugin `init`/`destroy` through a real gateway process:
- `wid == ngx.worker.id()` → one round per successful reload: `b destroy,a
destroy,a init,b init`
- `pid == ngx.worker.pid()` → two rounds, the second being exactly the
self-broadcast this check exists to skip
So with the suggestion applied every successful reload would tear the live
plugin set down and rebuild it twice on the serving worker.
--
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]
