AlinsRan commented on PR #13779:
URL: https://github.com/apache/apisix/pull/13779#issuecomment-5211869637
Force-pushed a reset of this branch. It now contains a single commit — the
cross-phase reuse of the filtered global-rule set — and no longer touches
`_M.filter()` at all.
The earlier revisions also added a fast path inside `_M.filter()` that
iterated the resource's configured plugins instead of scanning every loaded
one. I dropped it for two reasons:
1. **It broke auth plugins.** The fast path bypassed the slow path's loop,
and with it this bookkeeping:
```lua
if phase == "rewrite_in_consumer"
and (not plugin_conf._from_consumer or plugin_obj.type == "auth")
then
plugin_conf._skip_rewrite_in_consumer = true
end
```
Without that marking, auth plugins ran a second time after a consumer
merge, by which point the route's plugin config has been overwritten by the
consumer's — which is validated against `consumer_schema` and carries none of
the request-parsing fields. For jwt-auth that means `conf.header` is nil and
`core.request.header(ctx, nil)` aborts the request with a 500. That was the 108
failures in `t/plugin/jwt-auth.t` on the previous run. I had a fix, but the
underlying problem is that two copies of the same loop have to stay in sync
forever.
2. **Its remaining value is unmeasured.** The caching in this PR already
cuts `_M.filter()` calls per request from 9 to 1–2. How much a faster
`filter()` is still worth on top of that was never measured — the two numbers I
had came from different baselines and are not additive. Carrying the
ordering-equivalence burden for an unknown gain is not a good trade.
If it turns out `filter()` is still hot after this, I would rather submit it
separately, with the 2x2 measurement, and using a single index-driven path
instead of two parallel ones: collect the configured plugins' positions in
`local_plugins`, sort those integers, and walk one loop body. Since
`local_plugins` is already sorted, ascending positions reproduce its order
exactly — no comparator, and no special case for plugins that share a priority.
CI failures on the previous revision also included `nodejs.org` returning
502 during setup, unrelated to the code.
--
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]
