Repository navigation
fix: retain in-flight parameter counts across rule reloads - #3668
Draft
hutiefang76 wants to merge 2 commits into
Draft
hutiefang76 wants to merge 2 commits into
hutiefang76 wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
Reloading a parameter thread rule with a tighter limit can lose the counts of requests still running. With two entries in flight, replacing the old rules by a count-2 rule currently admits a third entry.
Does this pull request fix one issue?
Fixes #3667.
Describe how you did it
Use the newly loaded resource/index rules when deciding whether to retain the shared thread counter. Removed rules still lose their own time and token counters; the last-index and whole-resource cleanup paths remain intact. The existing public
clearForRule(rule)method keeps its behavior, and the metric does not depend on the manager.Describe how to verify it
On the #3657 baseline (
04e791b1), the eight real-entry regressions have two failures: the extra request is admitted after reload. The other six controls pass. With this follow-up, all eight pass, and the affected parameter-flow reactor and dependencies pass 274 tests on JDK 17, with no failures, errors or skips.mvn -B -pl sentinel-extension/sentinel-parameter-flow-control -am -Dsurefire.jdk-toolchain-version=17 testThe same eight regressions also pass on Zulu JDK 8 (XML runtime
1.8.0_492), using Maven on JDK 17 and a clean owned reactor:mvn -B -pl sentinel-extension/sentinel-parameter-flow-control -am clean -Dsurefire.jdk-toolchain-version=1.8 -Dtest=ParamFlowRuleManagerReloadTest -Dsurefire.failIfNoSpecifiedTests=false testverify -DskipTestspassed, including PMD checks with no violations; this verification does not count as another test run.Special notes for reviews
This is stacked on #3657 and stays in draft until that dependency lands. The three-file prerequisite comes from #3657; this follow-up changes
ParamFlowRuleManager, adds the package-private metric overload, and adds the real-entry reload regression class. The initialized-rule-sharing fix in #3657 is not new work in this PR.Implementation and review were AI-assisted. Local verification is separate from upstream CI and maintainer acceptance.