github-actions[bot] commented on code in PR #63298: URL: https://github.com/apache/doris/pull/63298#discussion_r3286032039
########## regression-test/suites/nonConcurrent/tablet_stat_mgr_p0/test_tablet_stat_mgr_concurrent_partition_churn.groovy: ########## @@ -0,0 +1,187 @@ +// 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 java.io.RandomAccessFile +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicReference + +suite("test_tablet_stat_mgr_concurrent_partition_churn", "nonConcurrent") { + String dbName = context.config.getDbNameByFile(context.file) + sql "select 1" + + String tableName = "test_tablet_stat_mgr_churn" + String oldInterval = null + AtomicBoolean stopped = new AtomicBoolean(false) + AtomicReference<Throwable> firstError = new AtomicReference<>() + // Track temp tables created by tableWorker so we can clean them up in finally. + AtomicInteger tableWorkerCount = new AtomicInteger(0) + + String dorisHome = System.getProperty("DORIS_HOME") + if (dorisHome == null || dorisHome.isEmpty()) { + dorisHome = context.config.suitePath.replace("/regression-test/suites", "") + } + + File feLog = new File("${dorisHome}/fe/log/fe.log") + File feWarnLog = new File("${dorisHome}/fe/log/fe.warn.log") + long feLogOffset = feLog.exists() ? feLog.length() : 0L + long feWarnLogOffset = feWarnLog.exists() ? feWarnLog.length() : 0L + + def readAppendedLog = { File file, long offset -> + if (!file.exists()) { + logger.warn("skip checking appended log because {} does not exist", file.getAbsolutePath()) + return "" + } + RandomAccessFile raf = new RandomAccessFile(file, "r") + try { + long safeOffset = Math.min(offset, raf.length()) + raf.seek(safeOffset) + int size = (int) (raf.length() - safeOffset) + if (size <= 0) { + return "" + } + byte[] bytes = new byte[size] + raf.readFully(bytes) + return new String(bytes, "UTF-8") + } finally { + raf.close() + } + } + + def failIfNeeded = { + if (firstError.get() != null) { + throw firstError.get() + } + } + + def startWorker = { Closure body -> + Thread.startDaemon { + try { + connect(context.config.jdbcUser, context.config.jdbcPassword, context.config.jdbcUrl) { + sql "use ${dbName}" + body() + } + } catch (Throwable t) { + logger.warn("tablet stat mgr concurrency worker failed", t) + firstError.compareAndSet(null, t) + stopped.set(true) + } + } + } + + try { + def configRow = sql """ ADMIN SHOW FRONTEND CONFIG LIKE 'tablet_stat_update_interval_second' """ + oldInterval = configRow[0][1] + sql """ ADMIN SET FRONTEND CONFIG ("tablet_stat_update_interval_second" = "1") """ Review Comment: This does not actually make the already-running `TabletStatMgr` tick every second. `ADMIN SET FRONTEND CONFIG` only calls `setInterval()` for configs registered in `Env.configtoThreads`, and that map currently only contains `dynamic_partition_check_interval_seconds`, not `tablet_stat_update_interval_second`. So in a normal cluster the daemon keeps its constructor interval (usually 60s), while this test only churns for 12s and then waits 3s; the later `SHOW DATA` assertion only checks that rows are returned, not that a stat-manager cycle ran. As a result the test can pass without exercising the concurrent TabletStatMgr iteration/TOCTOU path this PR is supposed to protect. Please either register this config for the daemon interval, invoke/synchronize with the stat update path directly, or assert against a reliable signal that a TabletStatMgr cycle actually ran during the churn. -- 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]
