You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "unbridled-41 (via GitHub)" <gi...@apache.org> on 2026/10/09 18:46:03 UTC
[PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
unbridled-41 opened a new pull request, #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930
### Which Issue(s) This PR Fixes
Fixes #5929
### Problem / Evidence
`createConsumerGroup` writes a fresh config with the creation defaults for every non-import call (`importing` is only true for the import endpoints):
```java
// before
config.setConsumeEnable(true); config.setConsumeBroadcastEnable(true); config.setRetryQueueNums(1);
...
boolean physicalExists = importing && verifyGroupImport(admin, brokerAddrs, config);
if (!physicalExists) { for (String addr : brokerAddrs) { admin.createAndUpdateSubscriptionGroupConfig(addr, config); } }
```
so calling create for a group that already exists on that broker reset `retryQueueNums` to 1, re-enabled consumption and broadcast consumption, and turned ordered consumption back to concurrent. Reachable from a resubmitted create form, and by design from the AI upsert, which calls create whenever the metadata table has no row - including for a physically existing group that was never imported.
```
RocketMQAdminClientImplTest#createConsumerGroupKeepsTheExistingBrokerSettingsOfAnAlreadyPresentGroupTest
expected: 5
but was: 1
```
The update path already does the opposite and documents it: "Read every broker before writing so missing or unreadable configurations cannot cause a partial update or fall back to creation defaults" / "Preserve each broker's other settings; zero is an explicit retry limit".
### Root cause / Fix
The create path had no notion of "already there". Pre-read the master's config, apply the retry limit and - only when the request names a delivery order type - the ordered flag, and keep everything else; absent groups and absent configs keep the creation defaults. The import path is unchanged.
### Priority and scoring
**PRIORITY 52** — impact 20/40 (silent reset of a live group's consumption switches and retry queues, i.e. a disabled group starts consuming again), blast radius 10/20 (any re-create of an existing group), reproducibility 18/20 (deterministic broker interaction, pinned by the new test), maintenance value 8/20 (aligns two paths that should agree).
**FIX_CONFIDENCE 80** — the pattern already exists in `updateConsumerGroup`; the ordering of the read is the only new logic.
### Tests
`cd server && mvn -o -B -ntp test -Dtest='RocketMQAdminClientImplTest'` -> `Tests run: 81, Failures: 0, Errors: 0`.
| Test | Before | After |
|---|---|---|
| `createConsumerGroupKeepsTheExistingBrokerSettingsOfAnAlreadyPresentGroupTest` | FAIL (`retryQueueNums` expected 5, got 1) | PASS |
The existing ordered/unordered create cases (`createConsumerGroupPropagatesAnOrderedDeliveryOrderTypeToTheBrokerTest`, `createConsumerGroupKeepsAnUnorderedDeliveryOrderTypeConcurrentTest`) still pass - they cover the absent-group path. `checkstyle:check` passes.
### Risk
The create path now performs one extra `examineSubscriptionGroupConfig` per master; a master that cannot be read falls back to the creation defaults exactly as before, and the import path keeps its own semantics.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "unbridled-41 (via GitHub)" <gi...@apache.org>.
unbridled-41 commented on PR #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930#issuecomment-6091469659
### Verification on the final commit (branch + commit + command results)
Every command from the description re-run on a fresh worktree of this branch (`git worktree add <dir> fork/<branch>`, no local modifications), at commit `039a030965`:
| command | exit | result |
| --- | --- | --- |
| `cd server && mvn -o -B -ntp test -Dtest='RocketMQAdminClientImplTest'` | 0 | `Tests run: 83, Failures: 0, Errors: 0, Skipped: 0 · BUILD SUCCESS` |
CI note: on this repository the `Backend Build` (binary-license-gate), `Frontend Build` / `Frontend Docker Build` (licence manifest) and `rmqctl Build` (YAML renderer quoting) jobs are red for every branch, trunk included; the module-level commands above are unaffected.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930#issuecomment-6330331913
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated changes.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5930: fix(group): keep an existing group's broker settings when create is called again
URL: https://github.com/apache/rocketmq-dashboard/pull/5930
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "unbridled-41 (via GitHub)" <gi...@apache.org>.
unbridled-41 commented on PR #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930#issuecomment-6091314863
### Verification on the final commit (branch + commit + command results)
Every command from the description re-run on a fresh worktree of this branch (`git worktree add <dir> fork/<branch>`, no local modifications), at commit `8d2b1ebee8`:
| command | exit | result |
| --- | --- | --- |
| `cd server && mvn -o -B -ntp test -Dtest='RocketMQAdminClientImplTest'` | 0 | `Tests run: 81, Failures: 0, Errors: 0, Skipped: 0 · BUILD SUCCESS` |
CI note: on this repository the `Backend Build` (binary-license-gate), `Frontend Build` / `Frontend Docker Build` (licence manifest) and `rmqctl Build` (YAML renderer quoting) jobs are red for every branch, trunk included; the module-level commands above are unaffected.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "unbridled-41 (via GitHub)" <gi...@apache.org>.
unbridled-41 commented on PR #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930#issuecomment-6091380320
### Follow-up review of this fix: the import path must not apply the creation defaults
Reviewing the first commit I found it changed the behaviour of the **import** path, not just the create path. `createConsumerGroup(admin, group, cluster, importing)` was applying `createConfigForMaster(...)` — the creation defaults (queue counts, permission, retention...) — to *every* broker also when `importing` was true. An import is documented to register a group that already matches and to refuse to overwrite a different config (`verifyGroupImport` throws 409); writing the defaults first meant an import of an existing, differently-configured group silently rewrote its live settings, and the 409 guard never got to run.
Commit `039a0309` restores that contract in this branch (first commit `8d2b1ebe` untouched, no force-push):
```java
if (importing) {
// An import only registers a group that already matches and refuses to overwrite a
// different config (verifyGroupImport throws 409); it never applies the creation
// defaults to a live group.
if (!verifyGroupImport(admin, brokerAddrs, config)) {
for (String addr : brokerAddrs) {
admin.createAndUpdateSubscriptionGroupConfig(addr, config);
}
}
} else {
for (String addr : brokerAddrs) {
admin.createAndUpdateSubscriptionGroupConfig(addr,
createConfigForMaster(admin, addr, config, group));
}
}
```
Two tests pin it in `RocketMQAdminClientImplTest`: `importConsumerGroupShouldNotRewriteAnExistingMatchingGroupTest` (a matching live group is left alone) and `importConsumerGroupShouldRefuseToOverwriteADifferentConfigTest` (an import over a different config still fails with the existing 409).
Verified at the current head (`039a0309`):
```
cd server && mvn -o -B -ntp test -Dtest='RocketMQAdminClientImplTest'
Tests run: 83, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```
Branch `fix/group-create-preserve-settings`, HEAD `039a0309` (on top of `8d2b1ebe`).
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(group): keep an existing group's broker settings when create is called again [rocketmq-dashboard]
Posted by "unbridled-41 (via GitHub)" <gi...@apache.org>.
unbridled-41 commented on PR #5930:
URL: https://github.com/apache/rocketmq-dashboard/pull/5930#issuecomment-6087717157
### CI status of this PR
`Backend Tests (Java 21)` — see the note below the list.
The four remaining red jobs are red for every branch, including `rocketmq-studio` itself:
- `Backend Build (Java 21)` — the `binary-license-gate` antrun step fails on a list of third-party jars (`spring-ai-*`, `swagger-ui`, `tencentcloud-sdk-*`, `zstd-jni`, ... : "missing complete upstream LICENSE/COPYING; a POM name or link is not a substitute"). This PR touches no build configuration; tracked by #5341.
- `Frontend Build (Node 20)` / `Frontend Docker Build` — the frontend licence manifest check fails the same way on branches that do not touch `web/` at all; tracked by #5346.
- `rmqctl Build (Go)` — `cmd` `TestMessageQueryCompletenessWarningTest` yaml subtests: `got {"items":[{"size":"3",...}]}, want {"size":3,...}`, i.e. the YAML renderer writes numbers as quoted strings where the test expects integer literals. Reproduced locally with `cd rmqctl && go test ./cmd/` from a checkout whose `rmqctl/` tree is byte-identical to `rocketmq-studio`'s, and owned by open PR #5850 ("keep yaml numbers unquoted").
`Backend Tests (Java 21)` failure attribution: `ResourceOwnershipGuardTest.migrationConcurrentStartupIsIdempotentTest` — `org.h2.jdbc.JdbcSQLNonTransientException: General error: java.util.ConcurrentModificationException` — the known flaky H2 concurrency test tracked by #5302. It reproduces independently of this diff (which touches no persistence code) and the same job passes on sibling PRs of the same batch.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org