github-actions[bot] commented on code in PR #66680:
URL: https://github.com/apache/doris/pull/66680#discussion_r3765732045
##########
be/src/exec/sink/writer/vtablet_writer.cpp:
##########
@@ -492,6 +492,32 @@ bool IndexChannel::_quorum_success(const
std::unordered_set<int64_t>& unfinished
}
}
+ const auto& table_sink = _parent->_t_sink.olap_table_sink;
+ if (table_sink.__isset.cross_az_succ_quorum) {
+ std::unordered_set<int64_t> finished_node_ids;
Review Comment:
The AZ gate needs tablet-level success here. A channel can finish
successfully while `result.tablet_errors()` has already put this node in
`_failed_channels[tablet_id]`; nevertheless this set adds the node for every
tablet. For example, with three completed az1 successes, one completed tablet
failure in az2, and one slow healthy az2 replica, ordinary quorum is genuinely
satisfied but `az2:1` is falsely considered satisfied. We then enter the
bounded post-quorum wait, can drop the healthy peer, and FE rejects a load that
could have succeeded by continuing the normal wait. Please exclude per-tablet
failures (and make the parallel v2 path require the tablet in
`success_tablets()` rather than just a finished stream), with tests for both
writers.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -541,6 +550,40 @@ public class Config extends ConfigBase {
+ "a load job.")
public static short min_load_replica_num = -1;
+ @ConfField(mutable = true, masterOnly = true, description = "Minimum
number of successfully written replicas "
+ + "required in each availability zone for a load job.")
+ public static String[] cross_az_succ_quorum = {};
+
+ public static Map<String, Integer> getCrossAzSuccQuorum() {
+ String[] config = cross_az_succ_quorum;
+ if (config == cachedCrossAzSuccQuorumConfig) {
+ return cachedCrossAzSuccQuorum;
+ }
+ synchronized (Config.class) {
+ config = cross_az_succ_quorum;
+ if (config == cachedCrossAzSuccQuorumConfig) {
+ return cachedCrossAzSuccQuorum;
+ }
+ Map<String, Integer> parsedConfig = new HashMap<>();
+ for (String item : config) {
+ String[] parts = item.split(":", -1);
+ try {
+ int configuredMin = Integer.parseInt(parts.length == 2 ?
parts[1].trim() : "");
+ if (parts[0].trim().isEmpty() || configuredMin < 0) {
+ throw new NumberFormatException();
+ }
+ parsedConfig.put(parts[0].trim(), configuredMin);
+ } catch (NumberFormatException e) {
Review Comment:
Malformed entries should make the config update fail atomically, not be
dropped here. Today `ADMIN SET ... ("cross_az_succ_quorum" =
"az1:2,az2:not-a-number")` succeeds, then commit enforces only az1; if every
item is invalid, the parsed map is empty and the AZ fence is completely
disabled. The added regression even codifies that fail-open behavior. Please
validate the entire value in the config callback, reject any
malformed/duplicate entry, and publish it only after full validation.
##########
gensrc/thrift/DataSinks.thrift:
##########
@@ -319,6 +319,7 @@ struct TOlapTableSink {
// initial partition list is empty, so auto-partition tables whose first
partitions arrive at
// runtime still enter the correct mode from the start.
25: optional bool enable_adaptive_random_bucket
+ 26: optional map<string, i32> cross_az_succ_quorum
Review Comment:
Making this optional preserves decoding, but not the feature's healthy-load
behavior during a rolling upgrade. An old coordinator BE ignores the map and
exits at ordinary quorum after its bounded grace; the PR's own 3-second
slow-AZ/1-second-grace case then loses the required replica, while the new FE
still enforces the AZ rule and rejects. Please fence nonempty activation until
every possible coordinator BE advertises support (or reject/defer it in a mixed
fleet), and add a new-FE/old-BE slow-AZ test.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/RemoteOlapTableSink.java:
##########
@@ -61,7 +60,7 @@ public TPaloNodesInfo createPaloNodesInfo() {
if (backend == null) {
continue;
}
- nodesInfo.addToNodes(new TNodeInfo(backend.getId(), 0,
backend.getHost(), backend.getBrpcPort()));
+ nodesInfo.addToNodes(createNodeInfo(backend));
Review Comment:
This cannot supply correct AZ data for remote writes.
`FeServiceClient.listBackends` builds these objects from `TBackend`, which has
no tag/location field, so every remote `Backend` keeps the `default` location;
inherited `setNodesInfo` also attaches the source FE's policy, while
`commitRemoteTxn` is validated under the target FE's policy. BE therefore
clamps target `az1`/`az2` requirements to zero and may abandon a slow target-AZ
replica before target FE rejects. Please carry the target policy and backend
locations in remote metadata/transaction setup and bind that snapshot to this
sink; add a two-cluster differing-policy test.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -541,6 +550,40 @@ public class Config extends ConfigBase {
+ "a load job.")
public static short min_load_replica_num = -1;
+ @ConfField(mutable = true, masterOnly = true, description = "Minimum
number of successfully written replicas "
+ + "required in each availability zone for a load job.")
+ public static String[] cross_az_succ_quorum = {};
+
+ public static Map<String, Integer> getCrossAzSuccQuorum() {
+ String[] config = cross_az_succ_quorum;
Review Comment:
This cache has no Java publication edge for a dynamic update.
`ConfigBase.setMutableConfig` reflectively writes the plain
`cross_az_succ_quorum` field under `ConfigBase.class`, while this
unsynchronized read (and the slow path's `Config.class` lock) uses neither that
monitor nor a volatile source reference; the setter also never writes the
volatile cache fields. A load may therefore keep taking the old identity/map
after an apparently successful enable. Please validate and publish one
immutable policy through a volatile/atomic reference (and add a concurrent
setter/reader test) instead of keying a cache off this unsafely published array.
--
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]
