github-actions[bot] commented on code in PR #66328:
URL: https://github.com/apache/doris/pull/66328#discussion_r3689496859
##########
be/src/exec/spill/spill_file.cpp:
##########
@@ -57,8 +69,15 @@ void SpillFile::gc() {
_spill_dir,
status.to_string());
}
}
- // decrease spill data usage anyway, since in ~QueryContext() spill data
of the query will be
- // clean up as a last resort
+
+ status = _data_dir->delete_empty_spill_query_dir(_spill_query_dir);
Review Comment:
[P2] Handle nested spill paths when pruning the query directory
The new contract only requires the relative path to start with a query-id
component, and `SpillRepartitioner::create_output_spill_files` already accepts
prefixes containing `/` (for example, `RouteBlockConservesTotalRowCount` writes
`test/route_block`). That creates `<root>/<query-id>/test/route_block_sub...`.
This GC removes only the leaf and then immediately calls
`rmdir(<root>/<query-id>)`, so the empty `test` directory makes the call return
`DIRECTORY_NOT_EMPTY` and the query directory is still leaked. Please either
reject nested suffixes at creation or prune empty ancestors up to
`_spill_query_dir` under the same synchronization, and cover this layout in the
GC test.
##########
be/test/vec/spill/spill_file_test.cpp:
##########
@@ -887,6 +888,101 @@ TEST_F(SpillFileTest, GCCleansUpFiles) {
auto st = io::global_local_filesystem()->exists(spill_file_dir, &exists);
ASSERT_TRUE(st.ok());
ASSERT_FALSE(exists);
+ st = io::global_local_filesystem()->exists(query_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_FALSE(exists);
+ st = io::global_local_filesystem()->exists(spill_root, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+}
+
+TEST_F(SpillFileTest, GCCleansUpEmptyQueryDirectoryWithoutDeletingSibling) {
+ SpillFileSPtr first_spill_file;
+ auto st = _create_and_write_spill_file("test_query/gc_first",
first_spill_file);
+ ASSERT_TRUE(st.ok()) << st;
+ SpillFileSPtr second_spill_file;
+ st = _create_and_write_spill_file("test_query/gc_second",
second_spill_file);
+ ASSERT_TRUE(st.ok()) << st;
+
+ auto query_dir = _data_dir_ptr->get_spill_data_path("test_query");
+ auto spill_root = _data_dir_ptr->get_spill_data_path();
+ auto first_spill_file_dir = query_dir + "/gc_first";
+ auto second_spill_file_dir = query_dir + "/gc_second";
+ auto second_part_file = second_spill_file_dir + "/0";
+
+ first_spill_file->gc();
+
+ bool exists = false;
+ st = io::global_local_filesystem()->exists(first_spill_file_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_FALSE(exists);
+ st = io::global_local_filesystem()->exists(query_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+ st = io::global_local_filesystem()->exists(second_spill_file_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+ st = io::global_local_filesystem()->exists(second_part_file, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+
+ // Repeated cleanup must not remove a sibling spill file.
+ first_spill_file->gc();
+ st = io::global_local_filesystem()->exists(second_part_file, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+
+ second_spill_file->gc();
+ st = io::global_local_filesystem()->exists(second_spill_file_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_FALSE(exists);
+ st = io::global_local_filesystem()->exists(query_dir, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_FALSE(exists);
+ st = io::global_local_filesystem()->exists(spill_root, &exists);
+ ASSERT_TRUE(st.ok());
+ ASSERT_TRUE(exists);
+}
+
+TEST_F(SpillFileTest, LazySpillFileRecreatesCleanedQueryDirectory) {
Review Comment:
[P2] Exercise the mkdir/rmdir race deterministically
This test recreates the query directory only after `first_spill_file->gc()`
has completed, so it still passes if `_spill_dir_mutex` is removed. The mutex
is the key part of the fix—the unsafe interleaving is parent `rmdir` racing
recursive child `mkdir`—but neither added test overlaps those operations.
Please add a deterministic concurrent BE-UT (for example with sync
points/barriers around the two `SpillDataDir` methods) that forces the
interleaving and verifies the writer's part survives and the final empty query
directory is removed.
--
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]
