Skip to content

Commit 551cc75

Browse files
committed
ct/l1: ignore extent timestamps when compaction lag is unset
The same defect as the preceding commit, in the L1 compaction source. Skip the timestamp check entirely when the lag is unset, as Kafka does. A negative lag is treated as unset too, mirroring Kafka's `Math.max(compactionLagMs, 0)`.
1 parent 9645653 commit 551cc75

2 files changed

Lines changed: 86 additions & 4 deletions

File tree

src/v/cloud_topics/level_one/maintenance/compaction/compaction_source.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,12 +119,14 @@ void align_extent_to_dirty_range(
119119
bool should_compact_extent(
120120
const metastore::extent_metadata& extent,
121121
std::chrono::milliseconds min_compaction_lag_ms) {
122+
// As in Kafka's LogCleanerManager::cleanableOffsets, the timestamp check is
123+
// skipped entirely when the lag is unset.
124+
if (min_compaction_lag_ms <= std::chrono::milliseconds{0}) {
125+
return true;
126+
}
122127
const auto now = to_time_point(model::timestamp::now());
123128
const auto max_extent_ts = to_time_point(extent.max_timestamp);
124-
if (now - max_extent_ts < min_compaction_lag_ms) {
125-
return false;
126-
}
127-
return true;
129+
return now - max_extent_ts >= min_compaction_lag_ms;
128130
}
129131

130132
} // namespace

src/v/cloud_topics/level_one/maintenance/compaction/tests/reducer_test.cc

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -959,6 +959,86 @@ TEST_F(ReducerTestFixture, MinCompactionLagMsReducerInterleavedTimestamps) {
959959
std::move(output_batches));
960960
}
961961

962+
// An extent's max timestamp may be ahead of the local clock: producers may set
963+
// create-time up to log_message_timestamp_after_max_ms (1h by default) in the
964+
// future, and the system clock may step backwards. With min.compaction.lag.ms
965+
// set, such an extent is held back until the clock catches up (as in Kafka's
966+
// `largestTimestamp > now - minCompactionLagMs`), but with the lag unset the
967+
// timestamp must not be consulted at all and the extent must still compact.
968+
TEST_F(ReducerTestFixture, MinCompactionLagMsUnsetTimestampsInFuture) {
969+
auto [ntp, tidp] = make_ntidp("test_topic");
970+
int num_produce_rounds = 4;
971+
int num_batches = 10;
972+
int num_records = 10;
973+
kafka::offset start_offset{0};
974+
kafka::offset last_offset{
975+
(num_produce_rounds * num_batches * num_records) - 1};
976+
auto gen = linear_int_kv_batch_generator();
977+
978+
const auto future_ts = model::timestamp(
979+
model::timestamp::now().value() + std::chrono::milliseconds{1h}.count());
980+
981+
for (int i = 0; i < num_produce_rounds; ++i) {
982+
model::test::record_batch_spec spec{
983+
.allow_compression = true,
984+
.count = num_records,
985+
.timestamp = future_ts,
986+
.all_records_have_same_timestamp = true};
987+
auto batches = gen(spec, num_batches);
988+
std::vector<tidp_batches_t> tidp_batches;
989+
tidp_batches.emplace_back(tidp, std::move(batches));
990+
make_l1_objects(std::move(tidp_batches)).get();
991+
}
992+
993+
auto info_spec = l1::metastore::compaction_info_spec{
994+
.tidp = tidp,
995+
.tombstone_removal_upper_bound_ts = model::timestamp::max()};
996+
997+
// With a lag configured, the future timestamps hold every extent back.
998+
auto compaction_info = _metastore.get_compaction_info(info_spec).get();
999+
ASSERT_TRUE(compaction_info.has_value());
1000+
ASSERT_TRUE(compaction_info->offsets_response.dirty_ranges.covers(
1001+
start_offset, last_offset));
1002+
do_compact(
1003+
tidp,
1004+
ntp,
1005+
std::move(compaction_info->offsets_response),
1006+
compaction_info->compaction_epoch,
1007+
compaction_info->start_offset,
1008+
&_metastore,
1009+
&_io,
1010+
/*min_compaction_lag_ms=*/1h)
1011+
.get();
1012+
1013+
compaction_info = _metastore.get_compaction_info(info_spec).get();
1014+
ASSERT_TRUE(compaction_info.has_value());
1015+
ASSERT_TRUE(compaction_info->offsets_response.dirty_ranges.covers(
1016+
start_offset, last_offset));
1017+
1018+
// With the lag unset, the same extents compact despite the future
1019+
// timestamps.
1020+
do_compact(
1021+
tidp,
1022+
ntp,
1023+
std::move(compaction_info->offsets_response),
1024+
compaction_info->compaction_epoch,
1025+
compaction_info->start_offset,
1026+
&_metastore,
1027+
&_io,
1028+
/*min_compaction_lag_ms=*/0ms)
1029+
.get();
1030+
1031+
compaction_info = _metastore.get_compaction_info(info_spec).get();
1032+
ASSERT_TRUE(compaction_info.has_value());
1033+
ASSERT_FLOAT_EQ(compaction_info->dirty_ratio, 0.0);
1034+
ASSERT_TRUE(compaction_info->offsets_response.dirty_ranges.empty());
1035+
1036+
auto reader = make_reader(ntp, tidp);
1037+
auto output_batches = read_all(std::move(reader));
1038+
linear_int_kv_batch_generator::validate_post_compaction(
1039+
std::move(output_batches));
1040+
}
1041+
9621042
TEST_F(ReducerTestFixture, MaxCompactibleOffsetReducer) {
9631043
// This test verifies that compaction respects the max_compactible_offset
9641044
// boundary. We create multiple extents and set max_compactible_offset to

0 commit comments

Comments
 (0)