github-actions[bot] commented on code in PR #67358:
URL: https://github.com/apache/doris/pull/67358#discussion_r3901639511
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudColocatePlacement.java:
##########
@@ -40,25 +42,52 @@ public static long score(long grpId, long idx, long beId) {
.asLong();
}
- public static long pickBackendId(long grpId, long idx, long[]
candidateBeIds) {
- return pickBackendId(grpId, idx, candidateBeIds,
CloudColocatePlacement::score);
+ public static long[] buildPlacement(long grpId, long[] candidateBeIds, int
bucketNum) {
+ return buildPlacement(grpId, candidateBeIds, bucketNum,
CloudColocatePlacement::score);
}
- static long pickBackendId(long grpId, long idx, long[] candidateBeIds,
ScoreFunction scoreFunction) {
+ static long[] buildPlacement(long grpId, long[] candidateBeIds, int
bucketNum, ScoreFunction scoreFunction) {
Preconditions.checkArgument(candidateBeIds.length > 0);
+ Preconditions.checkArgument(bucketNum > 0);
long[] sortedBeIds = Arrays.copyOf(candidateBeIds,
candidateBeIds.length);
Arrays.sort(sortedBeIds);
- long pickedBeId = sortedBeIds[0];
- long maxScore = scoreFunction.score(grpId, idx, pickedBeId);
- for (int i = 1; i < sortedBeIds.length; i++) {
- long beId = sortedBeIds[i];
- long score = scoreFunction.score(grpId, idx, beId);
- if (score > maxScore || (score == maxScore && beId < pickedBeId)) {
- maxScore = score;
- pickedBeId = beId;
+ int[] remainingQuota = new int[sortedBeIds.length];
+ Arrays.fill(remainingQuota, bucketNum / sortedBeIds.length);
+ int[] extraQuotaCandidates = new int[sortedBeIds.length];
+ Arrays.fill(extraQuotaCandidates, 1);
+ for (int i = 0; i < bucketNum % sortedBeIds.length; i++) {
+ int pickedIndex = pickBackendIndex(grpId, EXTRA_QUOTA_SCORE_IDX,
sortedBeIds,
+ extraQuotaCandidates, scoreFunction);
+ remainingQuota[pickedIndex]++;
+ extraQuotaCandidates[pickedIndex] = 0;
+ }
+
+ long[] placement = new long[bucketNum];
+ for (int idx = 0; idx < bucketNum; idx++) {
+ int pickedIndex = pickBackendIndex(grpId, idx, sortedBeIds,
remainingQuota, scoreFunction);
Review Comment:
[P1] Preserve low-remap behavior near one bucket per backend
The sequential quota consumption causes avoidable incumbent-to-incumbent
cascades. With `grpId=30`, 16 buckets, and backend IDs 1..15, adding BE 16
changes buckets 0, 2, 6, 8, 10, 13, 14, and 15 (8/16). Before scale-out only BE
7 has quota two; afterward every BE has quota one, so moving one BE-7 bucket to
BE 16 is sufficient and the other seven moves are avoidable. This contradicts
the stated low-remap HRW goal and can cold half of a valid colocate group on a
one-BE scale-out. Please use a bounded-load construction that preserves
incumbent assignments when the new quota vector permits it, and add a
16-bucket/15-to-16-BE regression.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java:
##########
@@ -189,10 +189,9 @@ private Backend
pickColocatedBackendForDeadGrace(CloudSystemInfoService infoServ
if (!Config.enable_cloud_colocate_consistent_hash) {
return pickColocatedBackend(infoService, groupId, clusterId,
availableBes);
}
- int bucketNum = infoService.getCloudColocateBucketsNum(groupId);
- CloudSystemInfoService.checkCloudColocateBucketIdx(groupId, clusterId,
idx, bucketNum);
- long[] availableBeIds =
availableBes.stream().mapToLong(Backend::getId).toArray();
- long pickedBeId = CloudColocatePlacement.pickBackendId(groupId.grpId,
idx, availableBeIds);
+ List<Long> availableBeIds =
availableBes.stream().map(Backend::getId).collect(Collectors.toList());
+ long pickedBeId = infoService.getCloudColocateHrwBeIdForDeadGrace(
Review Comment:
[P1] Balance the effective dead-grace placement, not each array separately
This call builds a quota-balanced placement for the full grace candidate
set, but `getColocatedBeId` keeps its live owners and consults the separately
balanced normal placement only at dead-owned indices. Those arrays do not
compose to a balanced live result. For `grpId=73`, 120 buckets, BEs `[1,2,3]`,
and recently-dead BE 2, the grace array is 40/40/40 and the `[1,3]` array is
60/60, but the 40 fallback indices split 11/29, so effective routing is 51/69
for the default one-hour grace window. That recreates the hot-backend condition
this PR is meant to remove. Please construct/cache the effective grace mapping
jointly using the live backends' residual quotas, and add an aggregate test
across all bucket indices.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java:
##########
@@ -224,10 +233,7 @@ private ColocatePlacementCache(long fingerprint, int
bucketNum, long[] beIdByBuc
private static ColocatePlacementCache build(long fingerprint, long[]
candidateBeIds, long grpId,
int bucketNum) {
- long[] beIdByBucket = new long[bucketNum];
- for (int i = 0; i < bucketNum; i++) {
- beIdByBucket[i] = CloudColocatePlacement.pickBackendId(grpId,
i, candidateBeIds);
- }
+ long[] beIdByBucket = CloudColocatePlacement.buildPlacement(grpId,
candidateBeIds, bucketNum);
Review Comment:
[P1] Stage this placement change across FE upgrades
This replaces the existing default-on HRW implementation in place, so old
and new FEs compute different owners from identical metadata during a rolling
upgrade. With `grpId=100`, six buckets, and BEs `[1,2,3]`, the old code maps
`[3,3,2,3,1,1]` while this builder maps `[3,3,2,2,1,1]`, disagreeing at bucket
3. `enable_cloud_colocate_consistent_hash` is restart-only and defaults to
true, so there is no knob that can keep the old behavior until every FE has
upgraded. Because requests and `SHOW TABLETS` can run locally with
`forward_to_master=false`, this also violates the existing three-FE
`BackendId`/`PrimaryBackendId` consistency check. Please introduce a separately
staged/versioned mode (or an equivalent cluster-version gate) and add old/new
coexistence coverage.
--
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]
