Conversation
Cache table-model source matches by database and table name instead of device id.\n\nMatch each table once per event and keep table-model TsFile table names complete for privilege checks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18648 +/- ##
============================================
- Coverage 42.82% 42.82% -0.01%
Complexity 442 442
============================================
Files 5451 5451
Lines 395425 395430 +5
Branches 51805 51808 +3
============================================
+ Hits 169349 169351 +2
- Misses 226076 226079 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Caideyipi
left a comment
There was a problem hiding this comment.
Thanks for this optimization. The table-level cache is a useful direction. I left two inline comments, including one authorization-cache invalidation race that can affect correctness.
Could you add focused coverage for the changed matching behavior as well?
- Multiple devices from one table should invoke table matching only once.
- A multi-table TsFile should still record every table after all sources have matched.
- Ideally, a cache-invalidation/concurrent-refill test should demonstrate that stale authorization results cannot survive an invalidation.
| // Use full cache to avoid queue stuck and block insertion | ||
| protected final Map<IDeviceID, Set<PipeRealtimeDataRegionSource>> deviceToSourcesCache; | ||
| protected final Map<Pair<String, IDeviceID>, Set<PipeRealtimeDataRegionSource>> | ||
| protected final Map<Pair<String, String>, Set<PipeRealtimeDataRegionSource>> |
There was a problem hiding this comment.
[P2] Prevent stale authorization results from refilling this cache
AuthorityChecker.invalidateCache() currently clears this matcher before invalidating the authority cache. That leaves a stale-refill window: a concurrent event can miss this cache, read the old authorization result, and cache a table-wide denial here. After the grant invalidation completes, later events can keep hitting that stale entry, remain unmatched, and advance progress until another invalidation or source change.
Please invalidate the authority cache before this matcher (also in invalidateAllCache()), or coordinate the two caches with a generation/version so that an entry computed before invalidation cannot be installed afterward.
| return new Pair<>(matchedSources, findUnmatchedSources(matchedSources)); | ||
| } | ||
|
|
||
| final String tableModelDatabaseName = |
There was a problem hiding this comment.
[P3] Resolve the table-model database name only for table events
getTableModelDatabaseName() is evaluated before the model check, so every insertion event, including tree-model events, now performs its lazy substring/toLowerCase initialization on the realtime write path even though the value is only used for table-model matching below. Please move this lookup into the table-model branch.
Invalidate the authority cache before the pipe matcher cache to avoid stale authorization refills.\n\nResolve the table-model database name only for table events and add matcher orchestration coverage.
|
Addressed in 77731ba.
The testCachedMatcher performance test is unchanged from master; its runtime is pre-existing. |
|
|
||
| public static boolean invalidateCache(String username, String roleName) { | ||
| final boolean invalidated = | ||
| authorityFetcher.get().getAuthorCache().invalidateCache(username, roleName); |
There was a problem hiding this comment.
[P1] Guard authority-cache refills with an invalidation generation
Reordering these invalidations only removes matcher entries produced by an in-flight match; it does not prevent the underlying authority cache from being refilled after its invalidation. A matcher miss holds the matcher read lock while checkCanSelectFromTable4Pipe() may issue a ConfigNode RPC. If a pre-revocation successful response returns after the author cache is cleared, ClusterAuthorityFetcher.checkPrivilegeFromConfigNode() can call putUserCache() with the old User while this thread is waiting for the matcher write lock. The following matcher invalidation then clears only the matcher entry, leaving the stale authority entry behind; the next event repopulates the matcher and a busy pipe can continue passing a revoked user.
Please add a generation/epoch to authority-cache loads (capture it before the RPC and only install the response if unchanged), or otherwise coordinate refills atomically with both invalidations. A latch-based test for this exact interleaving would prevent regression.
There was a problem hiding this comment.
Thanks for the detailed analysis. We understand the concern about authority-cache refill after invalidation.
This refill race is pre-existing and orthogonal to this PR: the unconditional putUserCache() path in ClusterAuthorityFetcher / BasicAuthorityCache is unchanged, and this PR is scoped to table-level matcher caching and its invalidation behavior. We do not plan to expand this PR into an authority-cache generation/epoch redesign.
If the maintainers consider it necessary, a separate PR can be opened later to handle authority-cache refill coordination and add the latch-based interleaving test there. For this PR, we would prefer to keep the matcher-related invalidation ordering fix and focused matcher tests.
Caideyipi
left a comment
There was a problem hiding this comment.
The table-level cache changes are directionally correct, but the authorization-cache refill race from thread #4032472791 remains a correctness blocker. An in-flight pre-invalidation ConfigNode response can still call putUserCache() after the invalidation has cleared the authority cache; a following pipe match can then cache that stale authorization result in the new per-table matcher. Reordering the two clears only narrows the timing and cannot prevent this interleaving. Please guard cache loads with an invalidation generation/epoch (or otherwise coordinate response installation with invalidation) and add the latch-based regression test before merge.
| final Set<PipeRealtimeDataRegionSource> matchedSources) { | ||
| ++tableMatchCount; | ||
| // Simulate a successful table-level match so this test focuses on match orchestration. | ||
| matchedSources.addAll(sources); |
There was a problem hiding this comment.
[P2] Make this test fail without table deduplication
With one registered source, matchedSources.addAll(sources) fills the set on the first table, so the matcher exits before it reaches the second device. The pre-change implementation therefore also reports tableMatchCount == 1, and this test would pass without the new tableNames.add(...) guard. Please register at least two sources and add only one (or otherwise keep the match set incomplete) so a duplicate invocation is observable.
Summary
Validation