lizhimins commented on PR #4581:
URL:
https://github.com/apache/rocketmq-dashboard/pull/4581#issuecomment-5761158708
Thanks — the underlying problem you identified is real, and the two tests
you added are both mutation-sensitive.
To restate it so we are clear about what is being decided: `UpdateTopicDTO`
already carries `writeQueues`/`readQueues` as `Integer` with `@PositiveOrZero`
and only copies them onto the VO when non-null, but `TopicVO` declares them as
primitive `int`, so "not submitted" arrives at
`RocketMQAdminClientImpl.updateTopic` as `0` and the `> 0` test reads it as
"not provided". An explicit drain-to-zero is therefore dropped.
**We are not taking this approach.** Our position is that the update path
should use the value the caller passed, as passed. This PR goes the other way:
it makes the VO nullable and then resolves the count through a three-way
substitution at every consumer — request value, else the stored `RmqTopic`
value, else a hardcoded 8. Each of those resolution points is a place where the
wrong number can be written to the broker, and the PR widens the number of
consumers that have to get the resolution right rather than removing the
ambiguity at the boundary. Concretely:
1. **The nullable VO leaks into contracts that were not updated.** Three AI
output records still take a primitive `int` and are constructed straight from
the VO: `TopicOutput`, `TopicListItem` and `TopicDetailOutput`. We traced every
caller and none can pass null today — but this PR is what makes null possible,
and the guards are caller discipline rather than construction. That is exactly
the "someone changes the wrong one later" failure mode.
2. **It changes the public REST response shape, which the description does
not say.** `AliyunInstanceProvider.createTopic` and `updateTopic` return the
caller's `TopicVO` verbatim and never populate the queue counts, and
`TencentInstanceProvider.updateTopic` only writes them back when a queue number
was requested. So a remark-only `POST /api/topics/update` against a cloud
instance would start serializing `"writeQueues": null` where trunk returns `0`.
`web/src/api/metadata.ts` declares `writeQueues: number` as non-nullable and
`topic.tsx` renders it directly. Note also that the topic form hides the queue
inputs entirely for cloud instances, so the UI never sends them there.
3. **The second commit carries an undisclosed behaviour change.** Flipping
`existing.getWriteQueueNums() > 0` to `>= 0` means a *stored* 0 now survives a
partial update instead of being reset to 8. That is an independent fix from the
request-side one, but the description presents it as "existing behaviour".
The drain-to-zero case is also only reachable from non-UI callers: the topic
edit form declares the counts `required` with `min={1}`, so Studio itself
cannot submit 0.
**What would make a future attempt welcome.** Keep `TopicVO` non-nullable
and settle the ambiguity at the API boundary instead, so the value that reaches
the broker is the value the caller sent: make the counts required on the update
DTO (they are already `Integer` + `@PositiveOrZero`) and confine the default-8
substitution to `createTopic`, where it belongs. If you would rather keep
partial updates, then the omitted case needs a representation that does not
depend on every downstream consumer remembering to null-check.
One mechanical note either way: #4538 has landed, so
`RocketMQAdminClientImpl.updateTopic` now throws 404 for a missing topic just
above the queue-count block. This branch needs a rebase onto `rocketmq-studio`,
and the `existing != null &&` guards become redundant once you do.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
