wenzhenghu commented on code in PR #68727: URL: https://github.com/apache/doris/pull/68727#discussion_r4267299273
########## fe/fe-core/src/main/java/org/apache/doris/common/profile/PlanningDiagnostics.java: ########## @@ -0,0 +1,437 @@ +// 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. + +package org.apache.doris.common.profile; + +import org.apache.doris.common.Config; +import org.apache.doris.common.util.DebugUtil; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.thrift.TUniqueId; + +import com.google.common.annotations.VisibleForTesting; +import com.google.gson.JsonObject; +import org.apache.logging.log4j.CloseableThreadContext; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.util.ArrayList; +import java.util.EnumMap; +import java.util.List; +import java.util.Locale; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.LongSupplier; +import java.util.function.Supplier; + +/** + * One planning pass. The planning thread owns timers and the bounded slow-operation buffer. + * The connection checker reads only immutable, volatile-published steps, never planner/table locks. + * Timings are inclusive: nested phases and operations must not be added to their enclosing phase. + */ +public final class PlanningDiagnostics { + private static final Logger LOG = LogManager.getLogger(PlanningDiagnostics.class); + private static final int MAX_SLOW_OPERATIONS = 16; + // Internal MV planning can replace ConnectContext while the calling planner still holds table locks. + private static final ThreadLocal<PlanningDiagnostics> ACTIVE = new ThreadLocal<>(); + + public enum Phase { + PREPROCESS, COLLECT_TABLES, PRELOAD_METADATA, WAIT_CHANGE_VISIBLE, LOCK_TABLES, + ANALYZE, REWRITE, PRE_REWRITE_MV, OPTIMIZE, CHOOSE_PLAN, POST_PROCESS, + TRANSLATE, DISTRIBUTE, RELEASE_RESOURCES; + + public String key() { + return name().toLowerCase(Locale.ROOT); + } + } + + private final ConnectContext context; + private final SummaryProfile summary; + private final long passId; + private final PlanningDiagnostics parent; + private final PlanningDiagnostics root; + private final boolean inheritsQueryIdentity; + private final TUniqueId queryId; + private final long statementId; + private final String threadName; + private volatile long timeoutMs; + private final LongSupplier clock; + private final long started; + private final EnumMap<Phase, Timing> timings = new EnumMap<>(Phase.class); + private final List<Event> slowOperations = new ArrayList<>(); + private final AtomicLong lastReport; + private volatile Step current; + private volatile Class<?> currentJob; + private volatile HeldLocks heldLocks = new HeldLocks(0, 0, ""); + private volatile boolean finished; + // Only the root publishes this pointer, so its registered connection can inspect an internal pass. + private volatile PlanningDiagnostics activePass; + private String failedStep = ""; + private Throwable stepFailure; + private long omittedOperations; + + private PlanningDiagnostics(ConnectContext context) { + this(context, System::nanoTime); + } + + @VisibleForTesting + PlanningDiagnostics(ConnectContext context, LongSupplier clock) { + this.context = context; + this.parent = ACTIVE.get(); + this.root = parent == null ? this : parent.root; + this.lastReport = parent == null ? new AtomicLong(Long.MIN_VALUE) : parent.lastReport; + SummaryProfile contextSummary = SummaryProfile.getSummaryProfile(context); + this.summary = contextSummary != null ? contextSummary : parent == null ? null : parent.summary; Review Comment: Addressed in [5d0a0779ce8d](https://github.com/apache/doris/commit/5d0a0779ce8db4a5f372cff13e1e07ba0d7b530a). A nested pass sharing its parent's SummaryProfile now updates separate nested-pass counts and the last nested breakdown, without adding its time/count to root totals. Root attempts still accumulate across retries/CTAS. `testNestedRewriteDoesNotInflateRootTotals` verifies one root pass and a rewrite total that includes the nested work only once. Nested-failure counts and independent nested Profile details are also covered. ########## fe/fe-core/src/main/java/org/apache/doris/common/profile/SummaryProfile.java: ########## @@ -582,6 +584,21 @@ public boolean isWarmup() { @ResetOnRetry(ResetScope.PLAN) private long waitChangeVisibleEndTime = -1L; + @SerializedName("planningPassCount") + private long planningPassCount; Review Comment: Adopted the smaller Audit surface in [5d0a0779ce8d](https://github.com/apache/doris/commit/5d0a0779ce8db4a5f372cff13e1e07ba0d7b530a): only `planning_passes`, `planning_failed_passes`, and `planning_last_failed` are added, after a root pass completes. Per-phase totals/failures remain in Profile. The code and PR description explicitly distinguish cumulative root-attempt summaries from legacy last-attempt timing keys. A retry test covers accumulation, and the real Audit regression with Profile disabled verified exactly three `planning_*` keys for successful and failed queries. Its `.out` was runner-generated and then verified in a second run. -- 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]
