AlinsRan commented on PR #13600:
URL: https://github.com/apache/apisix/pull/13600#issuecomment-5115471741
I pushed two commits to this branch (merged master to clear the conflict,
then fixed the tests). The plugin change itself is unchanged apart from the
conflict resolution.
**Conflict**: master added `_vid` handling to the key right where this PR
replaced the key construction, so that block would have been dropped. It now
lives inside `gen_limit_key()`, matching `limit-count`.
**Tests** — none of the assertions this PR touched were actually checking
the fix:
- `resource_key` is the full etcd key, so the expected value is
`/apisix/routes/1:<conf_version>:<key>`, not `routes/1:`. TEST 17 and TEST 19
were failing.
- `t/plugin/limit-conn-redis.t:591,683` still carried the pre-fix format and
were failing too.
- The two consumer blocks used `--- error_log_like`, which is not a
test-nginx section — those assertions were never evaluated. Changed to `---
error_log eval`.
- TEST 35-42 used `--- pipelined_requests`. That works for `limit-req` (rate
over a time window) but not for `limit-conn`: pipelined requests are handled
one after another and `_M.decrease()` runs in the log phase of each, so the
counter is back to zero before the next request starts. TEST 39 returned `[200,
200, 404]` and TEST 41 `[200, 200, 200, 200]`.
Those blocks are replaced by `t/plugin/limit-conn-shared-counter.t`, which
fires the requests in parallel via `ngx.thread.spawn` against an upstream that
sleeps, asserts the shared counter across two routes (`200,503`) and the
isolation between two consumers (`200,200`), and asserts the key is
`/apisix/consumers/jack:<conf_version>:jack`. Reverting the plugin change turns
the first case into `200,200`, so the test discriminates.
Locally `t/plugin/limit-conn.t`, `t/plugin/limit-conn-shared-counter.t`,
`t/stream-plugin/limit-conn.t` and `t/plugin/workflow3.t` pass.
One thing worth adding to the PR description: the cross-instance half of
#13584. `ctx.consumer_ver` is the local config-sync counter of
`core.config.new("/consumers")`, not an etcd revision, so the old key differed
per worker as well as per pod — with the `redis` policy the limit did not hold
across replicas either.
--
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]
