morningman commented on PR #67322:
URL: https://github.com/apache/doris/pull/67322#issuecomment-5509186668
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#67322
commit: 14d9bff34f75f40731ef6b6079a7ed8c99849fcc
base: 2b7475fa06a27437ae0384c9cf86d24fefd6407b
reviewed_at: 2026-09-02T20:00+08:00
reviewer: morningman
model: claude-opus-5[1m]
effort: max
findings: {blocker: 0, major: 0, minor: 4, nit: 2}
rounds: 1
converged: true
```
**Notes for maintainers**
-
`fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudClusterChecker.java:199-210`
— the sibling `NODE_STATUS_DECOMMISSIONED` branch still has both defects this
PR removes: `logBackendStateChange` sits outside the `setDecommissioned` CAS
(one journal write per 10s poll for as long as MS still lists the node), and
`:204` asserts on `isDecommissioned`, which nothing sets on the cloud path
before `:207`, so `"impossible status, somewhere has bug"` fires on every
normal decommission. `SystemHandler.java:603-607` has the idiom.
-
`fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudClusterChecker.java:190-195`
— the flag now serves as both the "watershed registered" latch and the "drain
traffic" switch, so while `getCurrentMaxTxnId` keeps failing the retiring BE
stays a legal migration destination (`CloudTabletRebalancer.java:1935-1936`);
previously that window was one tick. Self-healing, cannot wedge — but worth
recording as a deliberate trade-off.
-
`fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudClusterChecker.java:188`
— no test covers the once-only property and no Check List box is ticked.
`CloudUpgradeMgrTest` already has the reflection +
`Mockito.mockStatic(Env.class)` harness needed.
- Verified the two premises the fix rests on: `isDecommissioning` is not
persisted (`Backend.java:102` has no `@SerializedName`;
`GsonUtilsBase.java:92-95` skips such fields), so it shares one lifetime with
`txnBePairList`; and `registerWaterShedTxnId` mutates the queue only after its
throwing call, so "both or neither" holds.
- Not claimed in the PR body but true: for a single-BE compute group
`CloudTabletRebalancer.java:964` bypasses the `isActive`/tablet/WAL guard, so
the old code could send `NOTIFY_DECOMMISSIONED` after a *failed* registration
while the BE was still serving. This change removes that hazard.
<sub>Reviewed locally with the `doris-repo-review` pipeline (a local port of
`.github/workflows/code-review-runner.yml`). This is not a CI status
check.</sub>
<!-- doris-repo-review:v1:end -->
--
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]
