wuchong commented on code in PR #3340: URL: https://github.com/apache/fluss/pull/3340#discussion_r3292734395
########## fluss-server/src/main/java/org/apache/fluss/server/storage/DiskUsageCollector.java: ########## @@ -0,0 +1,95 @@ +/* + * 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.fluss.server.storage; + +import org.apache.fluss.annotation.Internal; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.io.IOException; +import java.nio.file.FileStore; +import java.nio.file.Files; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import static org.apache.fluss.utils.Preconditions.checkNotNull; + +/** + * Collects the local data disk usage ratio for the tablet server. The reported ratio is the + * <b>maximum</b> usage across all distinct {@link FileStore}s backing the configured data + * directories. A per-disk maximum (rather than a weighted average over total/used bytes) is used so + * that a single nearly-full disk cannot be masked by other low-usage disks in a multi-disk + * deployment: any single disk crossing the limit ratio must trip the write protection, because + * partitions pinned to that disk would otherwise fail to write. Multiple data directories sharing + * the same physical {@link FileStore} are still counted only once. + */ +@Internal +public final class DiskUsageCollector { + + private static final Logger LOG = LoggerFactory.getLogger(DiskUsageCollector.class); + + private final List<File> dataDirs; + + public DiskUsageCollector(List<File> dataDirs) { + checkNotNull(dataDirs, "dataDirs"); + this.dataDirs = Collections.unmodifiableList(dataDirs); + } + + /** + * Collects the current disk usage ratio in the range {@code [0.0, 1.0]}, defined as the maximum + * usage across all distinct {@link FileStore}s. Returns {@code 0.0} when no data directory is + * configured or every reachable file store reports a non-positive total space. + * + * <p>Individual directories that fail (e.g. deleted at runtime) are skipped with a warning so + * that one unhealthy directory does not prevent monitoring of the remaining disks. An {@link + * IOException} is thrown only when <b>all</b> directories fail. + */ + public double collect() throws IOException { + double maxRatio = 0.0; + Set<FileStore> counted = new HashSet<>(); + int failures = 0; + for (File dir : dataDirs) { + FileStore fs; + try { + fs = Files.getFileStore(dir.toPath()); + } catch (IOException e) { + LOG.warn("Failed to get FileStore for data directory {}; skipping.", dir, e); + failures++; + continue; + } + if (counted.add(fs)) { + long total = fs.getTotalSpace(); + if (total <= 0L) { + continue; + } + double ratio = (double) (total - fs.getUsableSpace()) / total; Review Comment: `collect()` only treats `Files.getFileStore()` failures as skippable. If `FileStore#getTotalSpace()` or `getUsableSpace()` throws for one data directory, the whole sample aborts instead of skipping just that directory, so `DiskUsageMonitor` can keep a stale lock state even when the other data dirs are still healthy and measurable. This should handle per-filesystem stat failures the same way as lookup failures. ########## fluss-server/src/main/java/org/apache/fluss/server/storage/LocalDiskManager.java: ########## @@ -402,6 +463,101 @@ public List<File> dataDirs() { return dataDirs; } + // ------------------------------------------------------------------------ + // Disk usage write protection + // ------------------------------------------------------------------------ + + /** + * Schedules the periodic disk usage sampling on the given {@link Scheduler}. An immediate + * synchronous sample is performed first so that the write-lock state is meaningful before the + * first scheduled tick. + */ + public void startDiskUsageMonitor(Scheduler scheduler) { + // initial sample to avoid an open window before the first scheduled run + diskUsageMonitor.runOnce(); + scheduler.schedule( + "disk-usage-monitor", + diskUsageMonitor::runOnce, + diskCheckIntervalMs, + diskCheckIntervalMs); Review Comment: nit: Setting `delayMs` to 0 can trigger immediate collection, rather than relying on an explicit invocation. This ensures the disk I/O operation executes asynchronously within the scheduler thread, preventing it from blocking the startup process. ########## fluss-server/src/main/java/org/apache/fluss/server/DynamicServerConfig.java: ########## @@ -64,7 +65,8 @@ class DynamicServerConfig { DATALAKE_FORMAT.key(), LOG_REPLICA_MIN_IN_SYNC_REPLICAS_NUMBER.key(), KV_SHARED_RATE_LIMITER_BYTES_PER_SEC.key(), - KV_SNAPSHOT_INTERVAL.key())); + KV_SNAPSHOT_INTERVAL.key(), + SERVER_DATA_DISK_WRITE_LIMIT_RATIO.key())); Review Comment: This key now passes the coordinator allowlist, but the range check still exists only in `LocalDiskManager.validate()`, which is registered on TabletServer, not CoordinatorServer. Values like `0.0` or `1.5` can therefore be persisted through `AlterConfigs` and only fail later when tablet servers try to apply them. The coordinator path should reject invalid `server.data-disk.write-limit-ratio` updates up front. I think we should also validate this on the Coordinator via `org.apache.fluss.server.DynamicConfigManager#registerValidator` by extending a `ConfigValidator`. We should also add an IT case for setting valid and invalid `server.data-disk.write-limit-ratio` (maybe near `FlussAdminITCase#testDynamicConfigs()`). -- 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]
