Skip to content

[studio][ISSUE #4201] Evict data-source cache after instance deletion - #4202

Merged
lizhimins merged 1 commit into
apache:rocketmq-studiofrom
89799969:codex/instance-data-source-cache-eviction
Sep 16, 2026
Merged

lizhimins merged 1 commit into
apache:rocketmq-studiofrom
89799969:codex/instance-data-source-cache-eviction

Conversation

@89799969

@89799969 89799969 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What is the purpose of the change

Fixes #4201.

After #4049, both full and paged data-source inventories are cached. InstanceService.deleteInstance updates DataSourceVO.instanceIds through SettingsRepository directly, bypassing the cache-eviction annotations in SettingsService. A successful deletion can therefore leave both cached inventories exposing the old instance binding.

This change invalidates the shared data-source cache only after the instance-deletion transaction commits. A rollback keeps the existing cache intact, and callers without active transaction synchronization retain the previous immediate-cleanup behavior.

Brief changelog

  • centralize the shared data-source cache name
  • clear that cache after a successful instance-deletion commit
  • keep endpoint/client cleanup on the same post-commit boundary
  • add regression coverage for binding removal, cache eviction, and commit timing

Verifying this change

  • Focused tests: InstanceServiceTest and SettingsServiceCachingTest
    • 84 tests run, 0 failures, 0 errors, 0 skipped
  • Checkstyle: 0 violations
  • The full server suite was also attempted, but this Windows host reported unrelated Java loopback-socket failures in HTTP/client tests plus two AuthCors assertions; no full-suite pass is claimed, and CI can provide clean-host coverage.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR correctly fixes a cache invalidation gap where deleting an instance left stale data-source bindings in the cache. The approach — evicting the shared cache after commit — is sound, and the constant extraction prevents future cache-name drift.

Findings

  • [Info] InstanceService.java:659 — Cache eviction ordering is safe (idempotent clear before endpoint release); worth a brief comment for future maintainers
  • [Info] SettingsService.java:59 — Good constant extraction; consider a one-line Javadoc explaining the cross-service dependency

Suggestions

No blocking concerns. The fix is minimal, well-targeted, and the regression test coverage is solid.


Automated review by github-manager-bot

@Override
public void afterCommit() {
evictDataSourceCache();
releaseApacheEndpointIfUnused(existing, null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Info] The cache eviction here is correctly placed in afterCommit(), ensuring the cache is only cleared after the deletion is durable. One consideration: if releaseApacheEndpointIfUnused throws, the cache has already been evicted but the endpoint release is incomplete. Since evictDataSourceCache() is idempotent (clearing an already-empty cache is a no-op), this ordering is safe — just worth noting for future maintainers.

@Service
public class SettingsService {

public static final String DATA_SOURCE_CACHE = "data-sources";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Info] Good extraction of the magic string into a constant. This prevents the cache-name drift that caused the original bug. Consider also adding a brief Javadoc on the constant explaining that it must match the @Cacheable/@CacheEvict values and that cross-service eviction depends on it.

Signed-off-by: halaxy <63827956+89799969@users.noreply.github.com>
@lizhimins
lizhimins force-pushed the codex/instance-data-source-cache-eviction branch from 1e9e349 to ce6312b Compare September 16, 2026 07:59
@lizhimins
lizhimins merged commit efcfcc4 into apache:rocketmq-studio Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants