github-actions[bot] commented on code in PR #65525:
URL: https://github.com/apache/doris/pull/65525#discussion_r3568766419
##########
fe/fe-core/src/main/java/org/apache/doris/qe/runtime/LoadProcessor.java:
##########
@@ -267,10 +266,8 @@ protected void
doProcessReportExecStatus(TReportExecStatusParams params, SingleF
*/
private boolean checkHealthy() {
for (SingleFragmentPipelineTask topFragmentTask : topFragmentTasks) {
- if (!topFragmentTask.isBackendHealthy(jobId)) {
Review Comment:
The restart check is still limited to `topFragmentTasks`, but `join()` is
waiting the full load latch built from every `(backendId, fragmentId)` in
`backendFragmentTasks`. In a multi-fragment insert, an upstream scan/shuffle
fragment can run on a different BE from the top sink fragment; if that upstream
BE restarts and loses its final report while the top-fragment BE stays alive,
this loop never calls `getBackendHealthStatus()` for the restarted BE, so the
full latch remains nonzero until `insert_timeout`. Please run the health/epoch
check over all unfinished load fragment tasks, or at least all distinct
backends represented in the full latch, and keep `topFragmentLatch` only for
the schedule-finish shortcut.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/runtime/SingleFragmentPipelineTask.java:
##########
@@ -62,12 +66,21 @@ public boolean
processReportExecStatus(TReportExecStatusParams reportExecStatus)
return this.done.compareAndSet(false, true);
}
- public boolean isBackendHealthy(long jobId) {
+ public Status getBackendHealthStatus(long jobId) {
if (backend.getLastMissingHeartbeatTime() > lastMissingHeartbeatTime
&& !backend.isAlive()) {
LOG.warn("backend {} is down while joining the coordinator. job
id: {}", backend.getId(), jobId);
- return false;
+ return new Status(TStatusCode.INTERNAL_ERROR, "backend {} is
down", backend.getId());
+ }
+
+ long currentProcessEpoch = backend.getProcessEpoch();
+ if (backendProcessEpoch != currentProcessEpoch && currentProcessEpoch
!= 0) {
+ Status unhealthyStatus = new Status(TStatusCode.INTERNAL_ERROR,
Review Comment:
This handles the current-epoch-zero compatibility case, but it still treats
a zero snapshot epoch as comparable. During upgrade/replay,
`Backend.getProcessEpoch()` can be `0` while the backend is still considered
alive/loadable; if this task is built in that window and the next heartbeat
fills in the same BE process's positive start time, `backendProcessEpoch !=
currentProcessEpoch` fires even though the BE did not restart after this load
began. Please only compare when both the captured and current epochs are
positive, and add the symmetric unit case for captured `0` followed by a
positive current epoch.
##########
regression-test/suites/load_p0/insert/test_insert_fail_fast_after_be_restart.groovy:
##########
@@ -0,0 +1,87 @@
+// 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 java.util.concurrent.TimeUnit
+
+suite("test_insert_fail_fast_after_be_restart", "docker") {
+ def options = new ClusterOptions()
+ options.enableDebugPoints()
+ options.setFeNum(1)
+ options.setBeNum(1)
+ options.cloudMode = false
+
+ docker(options) {
+ def tableName = "test_insert_fail_fast_after_be_restart"
+ GetDebugPoint().clearDebugPointsForAllBEs()
+
+ try {
+ sql "DROP TABLE IF EXISTS ${tableName}"
+ sql """
+ CREATE TABLE ${tableName} (
+ k BIGINT NOT NULL,
+ v BIGINT NOT NULL
+ )
+ DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1"
+ )
+ """
+
+ // Hold the load fragment before it can report completion to FE.
Restarting the BE
+ // at this point drops that report while the replacement process
quickly becomes alive.
+
GetDebugPoint().enableDebugPointForAllBEs("VTabletWriter.close.sleep",
[sleep_sec: 300])
+
GetDebugPoint().enableDebugPointForAllBEs("VTabletWriterV2.close.sleep",
[sleep_sec: 300])
+
+ def insertFuture = thread {
+ sql "SET enable_nereids_planner = true"
+ sql "SET enable_fallback_to_original_planner = false"
+ sql "SET insert_timeout = 300"
+ try {
+ sql """
+ INSERT INTO ${tableName}
+ SELECT number, number FROM numbers("number" = "1024")
+ """
+ return null
+ } catch (Throwable t) {
+ logger.info("INSERT failed after BE restart as expected:
${t.message}")
+ return t.message
+ }
+ }
+
+ // The small load reaches the writer close debug point well before
this wait ends.
+ sleep(3000)
+ cluster.restartBackends()
Review Comment:
This fixed sleep does not prove the insert has actually reached the
writer-close debug point before the BE is restarted. The debug points sleep
inside `VTabletWriter.close` / `VTabletWriterV2.close`, after earlier close
work, and the regression helper only enables the point; it does not wait until
the point has been hit. If docker CI is slow, `cluster.restartBackends()` can
happen before the blocked-close state, so the test can exercise the old
backend-down path or become flaky instead of proving the restarted-but-alive
epoch check. Please synchronize on an observable state/debug-point hit before
restarting, rather than sleeping a fixed 3 seconds.
--
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]
