github-actions[bot] commented on code in PR #66016: URL: https://github.com/apache/doris/pull/66016#discussion_r3664550062
########## regression-test/suites/partition_p1/auto_partition/sql/auto_partition_quorum_race_docker.groovy: ########## @@ -0,0 +1,98 @@ +// 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 + +suite("auto_partition_quorum_race_docker", "docker") { + def options = new ClusterOptions() + options.cloudMode = false + options.feNum = 1 + options.beNum = 3 + options.beConfigs += [ + "enable_quorum_success_write=true" + ] + + docker(options) { + // Exercise the normal scheduler. No preferred key, affinity mode, or + // non-default backend location tag participates in this test. + sql "set enable_load_backend_selection = false" Review Comment: [P1] Remove the unknown session-variable setup These three names are not registered session variables in this branch (an exact-name search finds them only in this test), and the fresh Docker FE does not whitelist them. `VariableMgr.setVar()` therefore raises `ERR_UNKNOWN_SYSTEM_VARIABLE` on this first `SET`, before either table is created, so the new regression never reaches the load-stream race. Please remove lines 30-41 and rely on the fresh cluster's defaults, or replace them with controls that actually exist in this branch; the source-tablet placement assertion below still verifies the intended topology. ########## be/src/exec/sink/writer/vtablet_writer_v2.cpp: ########## @@ -726,9 +726,23 @@ Status VTabletWriterV2::close(Status exec_status) { // close_wait on all non-incremental streams, even if this is not the last sink. // because some per-instance data structures are now shared among all sinks // due to sharing delta writers and load stream stubs. - // Do not need to wait after quorum success, - // for first-stage close_wait only ensure incremental streams load has been completed, - // unified waiting in the second-stage close_wait. + // + // This stage is also a cross-source fence for a source that has incremental streams: + // it must not close those streams before every other source has entered the close + // phase and can no longer open new incremental streams. + // + // A stream contributes to quorum only after CLOSE_LOAD has been sent + // (LoadStreamStub::is_closing) and the sender has observed both EOS and StreamClose. + // When this source has incremental streams, its non-incremental CLOSE_LOAD carries + // num_incremental_streams > 0, so the destination defers StreamClose until CLOSE_LOAD + // has arrived from all sources (LoadStream::_dispatch). Therefore, any non-incremental Review Comment: [P1] Make the receiver fence explicit before relying on `StreamClose` here `LoadStream::_dispatch` does not currently make EOS plus `StreamClose` a reliable all-source fence. Each accepted brpc stream has its own execution queue, so two `CLOSE_LOAD` callbacks can interleave: A's `close()` returns false; B's final callback returns true, appends and drains `_closing_stream_ids`; then A appends its id using the stale false result and is never closed. Conversely, the receiver sends `eos=true` before all sources close, and the sender's no-reason `on_closed` treats a transport reset like an orderly peer close, so EOS plus a reset can satisfy this fence too early. The new gate can therefore hang or advance early. Please publish an explicit successful all-original-sources-closed acknowledgement/state under `_lock` (and close late inserters from the protected global state), require that signal for first-stage votes, and add receiver interleaving/reset tests. -- 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]
