github-actions[bot] commented on code in PR #67498:
URL: https://github.com/apache/doris/pull/67498#discussion_r3930330102
##########
be/src/exec/spill/spill_file_manager.cpp:
##########
@@ -241,68 +242,96 @@ void
SpillFileManager::_retry_pending_query_spill_directories() {
}
void SpillFileManager::gc(int32_t max_work_time_ms) {
- bool exists = true;
bool has_work = false;
int64_t max_work_time_ns = max_work_time_ms * 1000L * 1000L;
MonotonicStopWatch watch;
watch.start();
+ // One summary line per spill store, printed together with the inode usage
of each store so
+ // that an inode leak or a growing gc backlog can be diagnosed from the BE
log alone.
+ std::vector<std::string> store_summaries;
Defer defer {[&]() {
if (has_work) {
std::string msg(
fmt::format("spill gc time: {}",
PrettyPrinter::print(watch.elapsed_time(),
TUnit::TIME_NS)));
msg += ", spill storage:\n";
- for (const auto& [path, store_dir] : _spill_store_map) {
- msg += " " + store_dir->debug_string();
+ for (const auto& summary : store_summaries) {
+ msg += " " + summary;
msg += "\n";
}
LOG(INFO) << msg;
}
}};
_retry_pending_query_spill_directories();
for (const auto& [path, store_dir] : _spill_store_map) {
- std::string gc_root_dir = store_dir->get_spill_data_gc_path();
+ SpillGcStats stats;
+ _gc_spill_store(store_dir.get(), watch, max_work_time_ns, &stats);
+ has_work |= stats.has_work;
+ store_summaries.emplace_back(fmt::format(
+ "{}, gc backlog: {} query dirs, deleted this round: {} dirs,
{} files, failed: {}",
Review Comment:
[P2] Count the production GC hierarchy represented by these fields
`init()` renames the entire active spill root to `spill_gc/<timestamp>`, so
the immediate entries counted by `backlog_dirs` are restart snapshots, not
query directories. Their children are query directories, and each is removed
recursively; a snapshot containing 100 queries and thousands of part files can
therefore log `backlog: 1 query dirs, deleted: 100 dirs, 0 files`, then still
report one query directory after all queries are gone because only the empty
timestamp wrapper remains. The new test builds the shallower
`spill_gc/<query>/<operator>/<part>` shape and never checks the summary, so it
misses this. Please either traverse/count the actual query/file levels
(including recursively reclaimed entries) or relabel these as top-level
snapshot/delete operations and cover the restart-shaped layout.
##########
be/src/exec/spill/spill_file_manager.cpp:
##########
@@ -241,68 +242,96 @@ void
SpillFileManager::_retry_pending_query_spill_directories() {
}
void SpillFileManager::gc(int32_t max_work_time_ms) {
- bool exists = true;
bool has_work = false;
int64_t max_work_time_ns = max_work_time_ms * 1000L * 1000L;
MonotonicStopWatch watch;
watch.start();
+ // One summary line per spill store, printed together with the inode usage
of each store so
+ // that an inode leak or a growing gc backlog can be diagnosed from the BE
log alone.
+ std::vector<std::string> store_summaries;
Defer defer {[&]() {
if (has_work) {
std::string msg(
fmt::format("spill gc time: {}",
PrettyPrinter::print(watch.elapsed_time(),
TUnit::TIME_NS)));
msg += ", spill storage:\n";
- for (const auto& [path, store_dir] : _spill_store_map) {
- msg += " " + store_dir->debug_string();
+ for (const auto& summary : store_summaries) {
+ msg += " " + summary;
msg += "\n";
}
LOG(INFO) << msg;
}
}};
_retry_pending_query_spill_directories();
for (const auto& [path, store_dir] : _spill_store_map) {
- std::string gc_root_dir = store_dir->get_spill_data_gc_path();
+ SpillGcStats stats;
+ _gc_spill_store(store_dir.get(), watch, max_work_time_ns, &stats);
+ has_work |= stats.has_work;
+ store_summaries.emplace_back(fmt::format(
+ "{}, gc backlog: {} query dirs, deleted this round: {} dirs,
{} files, failed: {}",
+ store_dir->debug_string(), stats.backlog_dirs,
stats.deleted_dirs,
+ stats.deleted_files, stats.failed_deletes));
+ }
+}
+
+void SpillFileManager::_gc_spill_store(SpillDataDir* store_dir, const
MonotonicStopWatch& watch,
+ int64_t max_work_time_ns, SpillGcStats*
stats) {
+ std::string gc_root_dir = store_dir->get_spill_data_gc_path();
+
+ std::error_code ec;
+ bool exists = std::filesystem::exists(gc_root_dir, ec);
+ if (ec || !exists) {
+ return;
+ }
+ // dirs of queries
+ std::vector<io::FileInfo> dirs;
+ auto st = io::global_local_filesystem()->list(gc_root_dir, false, &dirs,
&exists);
+ if (!st.ok()) {
Review Comment:
[P2] Include traversal failures in the new GC diagnostics
If this root `list()` returns EACCES/EIO, the function exits before
`has_work` is set, so no summary or warning is emitted. If the child `list()`
below fails, `backlog_dirs` has already been incremented but the status is
silently discarded, producing `failed: 0` even though GC could not inspect or
delete that backlog. Since this change is meant to make stuck GC observable,
please log/count scan failures (separately from delete failures if needed) and
retain the failing path/status; otherwise the new summary is most misleading on
the failure paths where it is needed.
##########
be/test/io/fs/local_file_system_test.cpp:
##########
@@ -283,6 +284,26 @@ TEST_F(LocalFileSystemTest, Exist) {
ASSERT_TRUE(check_exist(fname));
}
+TEST_F(LocalFileSystemTest, GetInodeInfo) {
+ size_t total = 0;
+ size_t available = 0;
+ auto st = io::global_local_filesystem()->get_inode_info(test_dir, &total,
&available);
+ ASSERT_TRUE(st.ok()) << st;
+ // Some file systems (e.g. btrfs) allocate inodes dynamically and report a
total of 0, so only
+ // the relation between the two values is portable.
+ EXPECT_GE(total, available);
+
+ struct statvfs vfs {};
+ ASSERT_EQ(::statvfs(std::string(test_dir).c_str(), &vfs), 0);
+ // The free inode count changes concurrently, so only cross-check the
total.
+ EXPECT_EQ(total, vfs.f_files);
Review Comment:
[P2] Avoid exact equality across two live inode snapshots
This compares separate `statvfs` samples, but `f_files` is not stable on
every supported local filesystem. XFS derives it partly from current free-block
state
([implementation](https://github.com/torvalds/linux/blob/master/fs/xfs/xfs_super.c)),
so unrelated allocation/free activity between these calls can fail the test
even when `get_inode_info()` is correct.
`SpillFileTest.UpdateCapacityTracksInodeUsage` repeats the same two-sample
equality and builds its expected debug text from the later value. Please use an
injected/single controlled sample, or restrict live-filesystem assertions to
invariants that remain valid across samples.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -387,6 +388,21 @@ Status LocalFileSystem::get_space_info_impl(const Path&
path, size_t* capacity,
return Status::OK();
}
+Status LocalFileSystem::get_inode_info(const Path& path, size_t* total,
size_t* available) {
+ FILESYSTEM_M(get_inode_info_impl(path, total, available));
+}
+
+Status LocalFileSystem::get_inode_info_impl(const Path& path, size_t* total,
size_t* available) {
+ struct statvfs vfs {};
+ if (::statvfs(path.c_str(), &vfs) != 0) {
+ return localfs_error(errno,
+ fmt::format("failed to get inode info for path
{}", path.native()));
+ }
+ *total = vfs.f_files;
Review Comment:
[P2] Normalize the all-ones unknown inode sentinel
Linux explicitly allows `f_files`/`f_ffree == -1`, and glibc copies
`f_ffree` to `statvfs.f_favail`
([kernel](https://github.com/torvalds/linux/blob/master/fs/statfs.c),
[glibc](https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/internal_statvfs.c)).
Copying those unsigned all-ones values here causes the signed gauges to
receive an invalid negative value, while `debug_string()` casts it back and
prints `18446744073709551615`; if only one field is unknown, `total -
available` can also wrap and trigger a bogus percentage warning. Please
normalize zero/all-ones (and inconsistent `available > total`) to an explicit
unknown state before publication/arithmetic, and add a deterministic sentinel
test.
--
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]
