Skip to content

[type:refactor] refactor shenyu selector and rule cache - #4578

Merged
yu199195 merged 59 commits into
apache:masterfrom
moremind:refactor-cache-3
May 16, 2023
Merged

yu199195 merged 59 commits into
apache:masterfrom
moremind:refactor-cache-3

Conversation

@moremind

Copy link
Copy Markdown
Member

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

@codecov-commenter

codecov-commenter commented May 11, 2023 •

Copy link
Copy Markdown

Codecov Report

Merging #4578 (df8abd5) into master (05f80fc) will decrease coverage by 0.02%.
The diff coverage is 54.41%.

@@             Coverage Diff              @@
##             master    #4578      +/-   ##
============================================
- Coverage     67.44%   67.42%   -0.02%     
- Complexity     7959     8000      +41     
============================================
  Files          1095     1096       +1     
  Lines         31296    31529     +233     
  Branches       2813     2859      +46     
============================================
+ Hits          21108    21260     +152     
- Misses         8531     8596      +65     
- Partials       1657     1673      +16     
Impacted Files Coverage Δ
...apache/shenyu/admin/controller/RuleController.java 100.00% <ø> (ø)
...he/shenyu/admin/controller/SelectorController.java 100.00% <ø> (ø)
.../admin/model/event/dict/BatchDictDeletedEvent.java 0.00% <ø> (ø)
...odel/event/metadata/BatchMetaDataDeletedEvent.java 0.00% <ø> (ø)
...in/model/event/plugin/BatchPluginDeletedEvent.java 0.00% <ø> (ø)
...odel/event/resource/BatchResourceCreatedEvent.java 69.23% <ø> (ø)
...odel/event/resource/BatchResourceDeletedEvent.java 0.00% <ø> (ø)
.../admin/model/event/role/BatchRoleDeletedEvent.java 0.00% <ø> (ø)
.../admin/model/event/rule/BatchRuleDeletedEvent.java 0.00% <ø> (ø)
...henyu/admin/model/event/rule/RuleUpdatedEvent.java 0.00% <ø> (ø)
... and 34 more

... and 14 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@li-keguo
li-keguo requested a review from yu199195 May 13, 2023 13:20
@yu199195
yu199195 merged commit cc1afd9 into apache:master May 16, 2023
Aias00 pushed a commit that referenced this pull request Oct 17, 2026
…7643) (#7663)

* fix(plugin-base): route match-success logging through the logged flag

Two cases where the code contradicted itself.

#7643's scope asks for routine match-success diagnostics to sit behind DEBUG or the
explicit matching-log setting, without duplicating messages. printLog already sits
behind the setting: it checks selectorData.getLogged() and ruleData.getLoged() and
carries the selector or rule name. defaultMatchSelector and defaultMatchRule logged
the same event a second time with no switch at all, so a user who had not enabled
logging still received one INFO line per matched request while the cache-hit path
stayed quiet. Both lines arrived from the same refactor (#4578) that introduced
printLog, and both are unreachable-as-documented once printLog runs, so they are
removed rather than demoted -- demoting them to DEBUG would have left two DEBUG lines
describing one match, which is the duplication the scope warns against. The surviving
INFO line is the one that names the selector or rule.

ShenyuWebsocketClient#handleResult logged the entire raw frame at INFO with no switch
and no level check, emitted before the GsonUtils.fromJson call, so a malformed frame
was written out in full before the catch discarded it. The payload moves to DEBUG
unchanged and INFO carries only groupType, eventType, namespaceId and payload size,
logged after parsing so the group type is available. Parse failures keep their WARN.

The two removed lines cost 51 and 44 bytes of message text per match (95 bytes before
logback's line prefix) for a default plugin name; a deployment matching on every
request wrote that much per request to INFO regardless of the logged setting. That
figure is derived from the message constants, not measured, and I have no controlled
environment here to report a before/after rate, an allocation rate or a p99 -- the
repo has no JMH infrastructure and I don't have a bare-metal Linux box, so I am not
going to present numbers I can't defend.

Tests cover both directions of the setting and the routing result: two cases assert no
selector or rule line reaches INFO when the flag is false, two assert exactly one line
does reach INFO when it is true and that it is the name-carrying one, and one asserts
doExecute still receives the same selector and rule. Removing the two getLogged and
getLoged guards makes the disabled cases fail, so they do pin the behaviour. The
exactly-once assertions also pin the de-duplication. logback-classic is added with
test scope, following shenyu-plugin-wasm-api and shenyu-sync-data-http, since this
module had only slf4j-api on the test classpath.

No new configuration flag and no change to matching or routing logic.

Fixes #7643

* fix(sync): report websocket payload size in bytes

Review follow-ups on the log-noise change:
- ShenyuWebsocketClient: count the payload as UTF-8 bytes instead of
  UTF-16 chars, so the INFO line's "bytes" unit stays accurate for
  non-ASCII configuration content.
- LogCapture: drop the unused originalLevel field and its no-op restore
  (the logger level is never modified during capture), and add the
  missing trailing newline.

AbstractShenyuPluginTest 13/13, ShenyuWebsocketClientTest 34/34.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants