juicewcode commented on PR #6973:
URL: https://github.com/apache/shenyu/pull/6973#issuecomment-5527933619
> ## Review: fix: Fix scale rule cache entity inconsistency (#6623)
> Reviewed at head `ce35b5f`. The create-side change is good, but the
update-path stale-key bug I flagged earlier is still present, so I'm requesting
changes.
>
> **Fixed (good):** `create` now caches the same `scaleRuleDO` instance it
inserts (`addOrUpdateRuleToCache(scaleRuleDO)`, line 10) instead of rebuilding
a second `ScaleRuleDO`. This removes the double-build inconsistency where the
cached object could differ from the persisted row.
>
> **Still broken — update leaves a stale metricName key:** the `update` path
(lines 15-20) is now `addOrUpdateRuleToCache(after)` where `after =
buildScaleRuleDO(scaleRuleDTO)`. `ScaleRuleCache` is keyed by
`rule.getMetricName()` (`ruleCache.put(rule.getMetricName(), rule)`, see
`ScaleRuleCache.addOrUpdateRuleToCache`), and `getAllRules()` returns
`ruleCache.values()`. When a rule's `metricName` changes, `update` only puts
the rule under the **new** metricName key; the entry under the **old**
metricName key is never removed. `getAllRules()` therefore returns both the old
(stale) and the updated rule until a cache reload/restart — a correctness bug.
>
> **Requested fix:** in `update`, load the existing rule before the update
and remove the old metricName key when it differs, e.g.:
>
> ```java
> final ScaleRuleDO before =
scaleRuleMapper.selectByPrimaryKey(scaleRuleDTO.getId());
> final ScaleRuleDO after = ScaleRuleDO.buildScaleRuleDO(scaleRuleDTO);
> int rows = scaleRuleMapper.updateByPrimaryKey(after);
> if (rows > 0) {
> if (Objects.nonNull(before) && !Objects.equals(before.getMetricName(),
after.getMetricName())) {
>
scaleRuleCache.removeRulesFromCache(List.of(before.getMetricName()));
> }
> scaleRuleCache.addOrUpdateRuleToCache(after);
> }
> ```
>
> (Note: #6975 already contains exactly this removal; if you intend #6975 to
always land together with this PR, please state the dependency explicitly so
reviewers don't merge #6973 standalone — otherwise the stale-key bug ships on
its own.)
>
> Please add the old-key removal (or confirm #6975 will always precede this)
and I'll approve.
Thanks for the review.
I have added the requested fix in the update path. The existing scale rule
is loaded before the update, and when its
metricName changes, the old metricName cache entry is removed before
caching the updated rule.
--
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]
