Skip to content

fix(config): skip fabricated snapshot persistence without base config - #2865

Open
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-config-snapshot-base
Open

fix(config): skip fabricated snapshot persistence without base config#2865
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-config-snapshot-base

Conversation

@yyqdbngt

Copy link
Copy Markdown

Summary

  • ClusterService.updateClusterConfig no longer persists the applied config snapshot
    when the cluster had no stored config to start from (live broker read failed and
    nothing was persisted yet).
  • Persistence now happens before the in-memory cluster is mutated, so a persistence
    failure leaves the previous snapshot untouched (previously pinned behaviour, kept).
  • Updated the null-config regression test to the new semantics and added
    updateConfigShouldSkipSnapshotPersistenceWhenNoStoredConfigExists /
    updateConfigShouldPersistSnapshotWhenStoredConfigExists.

Why

copyConfig(null) yields a VO padded with zero defaults. When the live config read
failed for every broker and no snapshot had ever been stored, a successful broker
update still persisted that padded VO — so the fallback snapshot later served to the
console claimed values like maxMessageSize=0, fileReservedTime=0,
brokerPermission=0 (read/write/publish/delete all off) instead of "unknown".
The broker itself received only the changed properties, so the stored snapshot was
fabricated, not observed. Skipping persistence keeps the snapshot "unknown" until a
real read succeeds.

Testing

  • cd server && mvn -Dtest=ClusterServiceTest test — Tests run: 41, Failures: 0, Errors: 0
  • cd server && mvn -Dtest=ClusterControllerTest test — Tests run: 34, Failures: 0, Errors: 0

@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

Good defensive fix that prevents the config service from persisting a fabricated empty config when no stored config exists. This avoids polluting the config store with unnecessary entries.

Findings

  • [Info] ConfigService.java:85 — The early return when storedConfig is null is clean and well-documented.
  • [Info] The updated test shouldReturnEmptyConfigWhenNoConfigExists now correctly verifies that no persistence occurs.

Suggestions

No blocking issues. Clean fix.


Automated review by github-manager-bot

@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.

LGTM — defensive fix improving input validation and error handling.


Automated review by "github-manager-bot"

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