github-actions[bot] commented on code in PR #67911:
URL: https://github.com/apache/doris/pull/67911#discussion_r4245259552
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudComputeGroupMeta.java:
##########
@@ -245,28 +245,32 @@ public void checkProperties(Map<String, String>
inputProperties) throws DdlExcep
}
public void modifyProperties(Map<String, String> inputProperties) throws
DdlException {
- String balanceType = inputProperties.get(BALANCE_TYPE);
+ applyBalanceTypeRule(properties, inputProperties.get(BALANCE_TYPE));
+ }
+
+ // only async_warmup carries a timeout: drop it for other types, fill the
default when missing
+ private static void applyBalanceTypeRule(Map<String, String> target,
String balanceType) {
Review Comment:
[P2] Normalize mixed-case balance types before persisting timeout state.
Validation accepts `WITHOUT_WARMUP`, but this rule only matches lowercase
strings. Starting from async warmup with timeout 900, a balance-only ALTER to
`WITHOUT_WARMUP` now sends and persists both the uppercase type and stale 900
through the full-map request. After FE restart, switching back to async without
an explicit timeout keeps 900 instead of the default 300; the old partial
request did not persist that stale timeout. Canonicalize the enum value in this
rule and cover the mixed-case round trip.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterComputeGroupCommand.java:
##########
@@ -87,8 +88,13 @@ public void validate(ConnectContext connectContext) throws
UserException {
public void doRun(ConnectContext ctx, StmtExecutor executor) throws
Exception {
validate(ctx);
CloudSystemInfoService cloudSys = ((CloudSystemInfoService)
Env.getCurrentSystemInfo());
+ // MS replaces the whole property map, so send current (timeout rule
already applied
+ // by validate) overlaid with the user input, not just the changed keys
+ Map<String, String> merged = new LinkedHashMap<>(
Review Comment:
[P2] Snapshot the property map without racing another ALTER. `validate()`
still mutates the shared `LinkedHashMap` in place when it removes or adds the
timeout, while this new copy iterates that map after the lookup lock has been
released. Concurrent ALTER commands can therefore make a valid command fail
with `ConcurrentModificationException`; `volatile` on the map reference does
not protect its contents. Use an immutable snapshot or synchronize the local
mutation and copy.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterComputeGroupCommand.java:
##########
@@ -87,8 +88,13 @@ public void validate(ConnectContext connectContext) throws
UserException {
public void doRun(ConnectContext ctx, StmtExecutor executor) throws
Exception {
validate(ctx);
CloudSystemInfoService cloudSys = ((CloudSystemInfoService)
Env.getCurrentSystemInfo());
+ // MS replaces the whole property map, so send current (timeout rule
already applied
+ // by validate) overlaid with the user input, not just the changed keys
+ Map<String, String> merged = new LinkedHashMap<>(
+
cloudSys.getComputeGroupByName(computeGroupName).getProperties());
+ merged.putAll(properties);
Review Comment:
[P2] Avoid copying stale FE properties into a second ALTER. On the master,
start with async warmup and timeout 900, then successfully ALTER only the
timeout to 300. That RPC updates MS, but `modifyProperties()` leaves FE's
timeout at 900 until the periodic checker runs. An immediate balance-only
`async_warmup` ALTER now copies stale 900 into `merged` and persists it again.
The prior balance-only request left timeout absent in MS, so a restarted FE
inherited 300; with this change it reloads 900. Update the FE mirror after
success or use an atomic MS-side property patch/version check.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterComputeGroupCommand.java:
##########
@@ -87,8 +88,13 @@ public void validate(ConnectContext connectContext) throws
UserException {
public void doRun(ConnectContext ctx, StmtExecutor executor) throws
Exception {
validate(ctx);
CloudSystemInfoService cloudSys = ((CloudSystemInfoService)
Env.getCurrentSystemInfo());
+ // MS replaces the whole property map, so send current (timeout rule
already applied
+ // by validate) overlaid with the user input, not just the changed keys
+ Map<String, String> merged = new LinkedHashMap<>(
+
cloudSys.getComputeGroupByName(computeGroupName).getProperties());
Review Comment:
[P2] Keep inherited defaults out of the MS replacement map. For a group with
only `balance_type=sync_warmup` in MS, a balance-only ALTER to `async_warmup`
adds FE's default timeout 300 and this copy persists it as an explicit MS
property. If the mutable global timeout later changes to 600 and FE restarts,
the rebalancer treats stored 300 as a group override and keeps using 300. The
former sparse request would inherit 600. Preserve which keys were explicit, or
patch the requested keys atomically in MS.
--
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]
