Copilot commented on code in PR #67379:
URL: https://github.com/apache/doris/pull/67379#discussion_r3903016561
##########
be/src/storage/compaction/compaction.cpp:
##########
@@ -104,6 +104,23 @@ using std::vector;
namespace doris {
using namespace ErrorCode;
+bool should_cache_cloud_cumulative_compaction_output() {
+ return !config::enable_file_cache_write_index_file_only;
+}
+
+bool should_cache_cloud_base_compaction_output(int64_t
input_rowsets_cached_size,
+ int64_t
input_rowsets_total_size) {
+ if (config::enable_file_cache_write_index_file_only) {
+ return false;
+ }
+ if (config::enable_file_cache_keep_base_compaction_output) {
+ return true;
+ }
+ return input_rowsets_total_size > 0 &&
+ double(input_rowsets_cached_size) /
double(input_rowsets_total_size) >
+
config::file_cache_keep_base_compaction_output_min_hit_ratio;
Review Comment:
The cache-hit ratio comparison uses `double` division on `int64_t` sizes,
which can lose precision for large byte values (beyond ~2^53) and lead to
incorrect decisions. Prefer a comparison that avoids dividing large integers
(e.g., compare `cached_size` against `min_hit_ratio * total_size` using `long
double`, or use a safe integer-based approach if the config ratio can be
represented as a rational).
##########
be/src/storage/compaction/compaction.h:
##########
@@ -55,6 +55,14 @@ class CloudStorageEngine;
static constexpr int COMPACTION_DELETE_BITMAP_LOCK_ID = -1;
static constexpr int64_t INVALID_COMPACTION_INITIATOR_ID = -100;
+
+bool should_cache_cloud_cumulative_compaction_output();
+bool should_cache_cloud_base_compaction_output(int64_t
input_rowsets_cached_size,
+ int64_t
input_rowsets_total_size);
Review Comment:
The new policy helpers are declared without any brief contract documentation
(e.g., precedence rules such as index-only overriding other policies, expected
value ranges for `*_min_hit_ratio`, and boundary behavior when `total_size ==
0`). Adding short comments/docstrings here would make the extracted API safer
to reuse correctly, especially as parallel compaction work lands.
##########
be/src/storage/compaction/compaction.h:
##########
@@ -55,6 +55,14 @@ class CloudStorageEngine;
static constexpr int COMPACTION_DELETE_BITMAP_LOCK_ID = -1;
static constexpr int64_t INVALID_COMPACTION_INITIATOR_ID = -100;
+
+bool should_cache_cloud_cumulative_compaction_output();
+bool should_cache_cloud_base_compaction_output(int64_t
input_rowsets_cached_size,
+ int64_t
input_rowsets_total_size);
+bool should_enable_compaction_cache_index_only(bool write_file_cache,
ReaderType compaction_type,
+ bool enable_base_index_only,
+ bool enable_cumu_index_only);
Review Comment:
These newly introduced free functions are declared in a widely-included
public header, which expands the public surface area for what appears to be
internal policy logic. If these are not intended as stable APIs, consider
moving them to a more internal header (or a `detail` namespace), or making them
`static`/`private` utilities on `CloudCompactionMixin` (or a dedicated policy
class) to keep the public API smaller and reduce downstream coupling.
##########
be/test/storage/compaction/compaction_file_cache_test.cpp:
##########
@@ -347,6 +350,25 @@ TEST_F(CompactionFileCacheTest,
GlobalIndexFileOnlyTakesPrecedenceOverCompaction
EXPECT_EQ(index_opts.approximate_bytes_to_write, 0);
}
+TEST_F(CompactionFileCacheTest,
CloudCumulativeCacheFollowsGlobalIndexOnlyPolicy) {
+ config::enable_file_cache_write_index_file_only = false;
+ EXPECT_TRUE(should_cache_cloud_cumulative_compaction_output());
+
+ config::enable_file_cache_write_index_file_only = true;
+ EXPECT_FALSE(should_cache_cloud_cumulative_compaction_output());
+}
+
+TEST_F(CompactionFileCacheTest, CloudBaseCacheFollowsExistingPolicy) {
Review Comment:
This test implicitly depends on `SetUp()` forcing
`file_cache_keep_base_compaction_output_min_hit_ratio` to `0.5`. To make the
test robust to future fixture changes (or additional tests that tweak the
ratio), set the ratio explicitly within the test (and/or assert the ratio being
used) so the threshold assumptions are local and self-evident.
--
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]
