github-actions[bot] commented on code in PR #69554:
URL: https://github.com/apache/doris/pull/69554#discussion_r4369805659
##########
be/src/exec/sink/writer/vtablet_writer.cpp:
##########
@@ -136,6 +137,17 @@ Status IndexChannel::init(RuntimeState* state, const
std::vector<TTabletWithPart
if (tablet_locations == nullptr) {
return Status::InternalError("unknown tablet, tablet_id={}",
tablet.tablet_id);
}
+ int selected_replicas_num =
cast_set<int>(tablet_locations->node_ids.size());
+ int load_required_replicas_num =
_load_required_replicas_num(tablet.tablet_id);
+ if (selected_replicas_num < load_required_replicas_num) {
Review Comment:
[P1] Preserve the bootstrap tablet for empty automatic tables
With `enable_memtable_on_sink_node=false`, the first load into an empty
`AUTO PARTITION BY LIST (...) ()` table now fails even when every backend is
healthy. `OlapTableSink.createDummyPartition()` creates partition/tablet 0, and
`createDummyLocation()` intentionally gives it exactly one backend. The initial
sink still assigns it the default allocation's required quorum of two, and
`VOlapTablePartitionParam::init()` retains it in `_partitions` (the fake flag
only excludes its keys from lookup). Thus a normal three-replica table reaches
this check with selected=1, required=2 and returns `tablet 0 in partition 0 has
1 selected replicas, fewer than required 2` before real partitions can be
created.
Handle the synthetic tablet separately in initialization and failure
accounting while retaining quorum enforcement for real tablets. Add an
end-to-end first-load case using an empty automatic table and the legacy
writer; the new direct createPartition RPC tests do not exercise this bootstrap.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Tablet.java:
##########
@@ -240,12 +240,19 @@ protected Multimap<Long, Long>
getNormalReplicaBackendPathMapImpl(String beEndpo
}
// return map of (BE id -> path hash) of normal replicas
- // for load plan.
public Multimap<Long, Long> getNormalReplicaBackendPathMap() throws
UserException {
TabletSlidingWindowAccessStats.recordTablet(getId());
return getNormalReplicaBackendPathMapImpl(null, (rep, be) ->
rep.getBackendId());
}
+ // return map of (BE id -> path hash) of replicas eligible for a new load
plan
+ public Multimap<Long, Long> getLoadReplicaBackendPathMap() throws
UserException {
+ Multimap<Long, Long> pathMap = getNormalReplicaBackendPathMap();
+ SystemInfoService infoService = Env.getCurrentSystemInfo();
+ pathMap.entries().removeIf(entry ->
!infoService.checkBackendLoadAvailable(entry.getKey()));
Review Comment:
[P1] Account for filtered replicas in the V2 writer as well
The new filter also feeds `VTabletWriterV2`, but only legacy `IndexChannel`
received selected-replica failure accounting. With
`enable_memtable_on_sink_node=true`, replication=3, required=1, one-bucket
partitions A on BEs{1,2,3} and B on{1,4,5}, disabling loads on BEs2/3 leaves A
only BE1. If BE1 has a receiver-side rowset/delete-bitmap/commit failure during
CLOSE_LOAD, `IndexStream::close()` reports A in `failed_tablets` with overall
OK/EOS. Stream close still succeeds, and V2's `_create_commit_info()` tolerates
that one failure against configured3-required1=2. B succeeds through4/5, while
A produces no commit info; FE `checkCommitStatus()` consequently checks B only
and the load can succeed with every A row lost. The existing
`LoadStreamWriter.close.cancelled` point on BE1 provides a concrete late-close
trigger.
This is the separate V2 implementation, beyond the legacy-only instance in
thread 4367374261. Apply selected-destination accounting to V2 too, or require
successful participation of every touched partition, and add a V2
mixed-partition late-close failure regression.
##########
regression-test/suites/fault_injection_p0/test_disable_load_selected_replica_failure.groovy:
##########
@@ -0,0 +1,96 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.apache.doris.regression.suite.ClusterOptions
+import org.apache.doris.regression.util.NodeType
+
+suite("test_disable_load_selected_replica_failure", "docker") {
+ def options = new ClusterOptions()
+ options.cloudMode = false
+ options.feNum = 1
+ options.beNum = 5
+ options.feConfigs += ["disable_tablet_scheduler=true"]
+ options.enableDebugPoints()
+
+ docker(options) {
+ def backendIPs = [:]
+ def backendPorts = [:]
+ getBackendIpHttpPort(backendIPs, backendPorts)
+ def backendIds = backendIPs.keySet().sort { it.toLong() }
+ def sharedBackend = backendIds[0]
+ def disabledBackends = backendIds[1..2]
+ def tags = ["shared", "left", "left", "right", "right"]
+ backendIds.eachWithIndex { backendId, i ->
+ sql """ALTER SYSTEM MODIFY BACKEND "${backendId}" SET
("tag.location" = "${tags[i]}")"""
+ }
+
+ def originalMemtableOnSink = sql("select
@@enable_memtable_on_sink_node")[0][0]
+ sql "set enable_memtable_on_sink_node = false"
+ sql "drop table if exists disable_load_selected_replica_failure"
+ // Deterministic placement: A={1,2,3}, B={1,4,5}; both require one
successful replica.
+ sql """
+ create table disable_load_selected_replica_failure (k int, v int)
+ duplicate key(k)
+ partition by range(k) (
+ partition p_a values less than ("10"),
+ partition p_b values less than ("30")
+ ("replication_allocation" = "tag.location.shared: 1,
tag.location.right: 2")
+ )
+ distributed by hash(k) buckets 1
+ properties (
+ "replication_allocation" = "tag.location.shared: 1,
tag.location.left: 2",
+ "min_load_replica_num" = "1"
+ )
+ """
+
+ def point = "LoadChannel.add_batch.failed"
+ def debugPoint = GetDebugPoint()
+ try {
+ disabledBackends.each { backendId ->
+ sql """ALTER SYSTEM MODIFY BACKEND "${backendId}" SET
("disable_load" = "true")"""
+ }
+ debugPoint.enableDebugPoint(backendIPs[sharedBackend],
+ backendPorts[sharedBackend] as int, NodeType.BE, point)
+
+ // B still succeeds through BE4/5 while BE1's actual writer RPC
fails.
+ sql "insert into disable_load_selected_replica_failure values (21,
100)"
Review Comment:
[P2] Seed the baseline before enabling the shared-backend fault
This unqualified INSERT plans both A and B: absent an explicit PARTITION
clause, the sink expands its partition IDs to the whole table. The legacy
writer therefore opens A's tablet on BE1 and sends EOS to that node even though
this row belongs only to B. With the fault already enabled, BE1's add-batch
response cancels its channel; close handling calls `mark_as_failed(..., -1)`,
which also marks untouched A failed. A has selected=required=1, so its new
failure allowance is zero and this unwrapped setup statement errors before the
following `test { ... }` or either atomicity assertion is reached.
Insert the baseline before enabling the fault, or explicitly use
`PARTITION(p_b)` for this seed so only the intended mixed-partition statement
exercises A's failing destination.
--
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]
