gavinchou commented on PR #67911:
URL: https://github.com/apache/doris/pull/67911#issuecomment-6096233560
## Findings
- **[major] The full replacement request is built from a stale FE cache and
can overwrite a value that was already committed to Meta Service.**
`AlterComputeGroupCommand#doRun` copies
`CloudComputeGroupMeta.properties`, overlays only the user input, and sends the
result to `ALTER_PROPERTIES`. Meta Service then replaces the whole property
map. However, a successful ALTER does not publish the committed map back to FE;
FE is refreshed only by `CloudInstanceStatusChecker`, every 10 seconds by
default.
One concrete sequence is:
1. FE and MS contain `balance_type=async_warmup,
balance_warm_up_task_timeout=300`.
2. `ALTER ... (balance_warm_up_task_timeout=6000)` commits `6000` to MS,
while FE still holds `300`.
3. Before the checker runs, `ALTER ... (balance_type=async_warmup)` copies
the stale FE map and sends `timeout=300`, reverting the previous successful
update.
There is also an interleaving between `modifyProperties()` and
`setProperties()`: validation mutates the currently published map in place,
while the checker can replace the volatile map before `doRun` copies it. This
can produce a replacement such as `balance_type=without_warmup` together with a
timeout, even though the timeout rule should remove it. `volatile` only
publishes the reference; it does not make this multi-step read-modify-write
atomic.
Please perform the patch merge and validation against the authoritative MS
state in the MS transaction, ideally with a version/CAS. If this must remain in
FE, build and validate a candidate from one immutable/versioned snapshot, avoid
mutating the FE mirror before RPC success, and publish only the committed
candidate with generation protection. Please also add tests for consecutive
ALTERs before the checker runs, checker/ALTER interleavings, and the final RPC
request.
- **[minor] A concurrent drop or rename can turn a controlled not-found
error into an NPE.**
`validate()` checks that the group exists, but `doRun()` performs a second
`getComputeGroupByName(...).getProperties()` without a null check. The
background cluster checker can remove or rename the mapping between those
calls. The existing RPC path has a controlled `compute group not found` error,
but the new dereference fails before reaching it. Please carry a stable group
ID/generation through the operation or revalidate the lookup.
**Review decision: Request 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
