diff --git a/src/v/cloud_storage/remote_partition.cc b/src/v/cloud_storage/remote_partition.cc index a3c0cc7683bad..02964280c723c 100644 --- a/src/v/cloud_storage/remote_partition.cc +++ b/src/v/cloud_storage/remote_partition.cc @@ -20,6 +20,7 @@ #include "cloud_storage/tx_range_manifest.h" #include "cloud_storage/types.h" #include "model/fundamental.h" +#include "model/timestamp.h" #include "net/connection.h" #include "ssx/future-util.h" #include "ssx/watchdog.h" @@ -605,27 +606,23 @@ class partition_record_batch_reader_impl final [&](model::timestamp query_ts) { // Special case, it can happen when a timequery falls below // the clean offset. Caused when the query races with - // retention/gc. log a warning, since the kafka client can - // handle a failed query + // retention/gc. auto const& spillovers = _partition->_manifest_view ->stm_manifest() .get_spillover_map(); - if ( - spillovers.empty() - || spillovers.get_max_timestamp_column() - .last_value() - .value_or(model::timestamp::max()()) - >= query_ts()) { + + bool timestamp_inside_spillover + = query_ts() <= spillovers.get_max_timestamp_column() + .last_value() + .value_or(model::timestamp::min()()); + + if (timestamp_inside_spillover) { vlog( _ctxlog.debug, "Manifest query raced with retention and the result " "is below the clean/start offset for {}", query_ts); - return true; } - - // query was not meant for archive region. fallthrough and - // log an error return false; })) { // error was handled diff --git a/src/v/cloud_storage/tests/remote_partition_test.cc b/src/v/cloud_storage/tests/remote_partition_test.cc index 7bd90f8181108..caba7212298aa 100644 --- a/src/v/cloud_storage/tests/remote_partition_test.cc +++ b/src/v/cloud_storage/tests/remote_partition_test.cc @@ -2417,9 +2417,17 @@ FIXTURE_TEST(test_out_of_range_spillover_query, cloud_storage_fixture) { return what.find("Failed to query spillover manifests") != what.npos; }); - // BUG: This assertion is disabled because it currently fails. - // test_log.debug("Timestamp undershoots the partition"); - // BOOST_TEST_REQUIRE(timequery(*this, model::timestamp(100), 3 * 6)); + // Timestamp undershoots the partition. + // It shouldn't throw. But as an interim workaround we throw until + // clean offset catches up with start offset. + // The clients will retry. + BOOST_REQUIRE_EXCEPTION( + timequery(*this, model::timestamp(100), 3 * 6), + std::runtime_error, + [](const auto& ex) { + ss::sstring what{ex.what()}; + return what.find("Failed to query spillover manifests") != what.npos; + }); test_log.debug("Timestamp within valid spillover but below archive start"); BOOST_TEST_REQUIRE( @@ -2435,4 +2443,22 @@ FIXTURE_TEST(test_out_of_range_spillover_query, cloud_storage_fixture) { test_log.debug("Timestamp overshoots the partition"); BOOST_TEST_REQUIRE(timequery(*this, model::timestamp::max(), 0)); + + // Rewrite the manifest with clean offset to match start offset. + manifest.set_archive_clean_offset( + archive_start, manifest.archive_size_bytes() / 2); + vlog( + test_util_log.info, + "Rewriting manifest at {}:\n{}", + manifest.get_manifest_path(), + ostr.str()); + + remove_expectations({manifest_url}); + add_expectations({ + cloud_storage_fixture::expectation{ + .url = manifest_url, .body = serialize_manifest(manifest)}, + }); + + // This query should now succeed. + BOOST_TEST_REQUIRE(timequery(*this, model::timestamp(100), 3 * 6)); }