Fix timequery returning wrong offset after trim-prefix which could lead to stuck consumers #18112
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,8 +130,9 @@ static ss::future<list_offset_partition_response> list_offsets_partition( | |
| kafka_partition->leader_epoch()); | ||
| } | ||
| auto res = co_await kafka_partition->timequery(storage::timequery_config{ | ||
| kafka_partition->start_offset(), | ||
| timestamp, | ||
| model::prev_offset(offset), | ||
|
There was a problem hiding this comment. I'm wondering what the implications of this are. Presumably if the Maybe a good idea to add any implications (or lackthereof) into the commit message There was a problem hiding this comment.
You mean prior to this commit? It would have returned a potentially non-committed offset even with acks=all. Normally, redpanda doesn't expose such offsets unless write caching is in use. Fetch would have failed with out of range too, causing the consumer to reset. There was a problem hiding this comment.
@nvartolomei please include in commit message more detail. the commit by inspection implies an off-by-one error, but there are critically useful pieces of information a reader in the future may want. for example, how was it discovered, what are symptoms of it being wrong, etc... |
||
| kafka_read_priority(), | ||
| {model::record_batch_type::raft_data}, | ||
| octx.rctx.abort_source().local()}); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| // Copyright 2024 Redpanda Data, Inc. | ||
nvartolomei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.md | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0 | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "base/vassert.h" | ||
| #include "model/fundamental.h" | ||
|
|
||
| namespace model { | ||
| /// A non-empty, bounded, closed interval of offsets [min offset, max offset]. | ||
| /// | ||
| /// This property helps to simplify the logic and the instructions required to | ||
| /// check for overlaps, containment, etc. It is the responsibility of the caller | ||
| /// to ensure these properties hold before constructing an instance of this | ||
| /// class. | ||
| /// | ||
| /// To represent a potentially empty range, wrap it in an optional. | ||
| class bounded_offset_interval { | ||
| public: | ||
| static bounded_offset_interval | ||
| unchecked(model::offset min, model::offset max) noexcept { | ||
| return {min, max}; | ||
| } | ||
|
|
||
| static bounded_offset_interval | ||
| checked(model::offset min, model::offset max) { | ||
| if (min < model::offset(0) || max < model::offset(0) || min > max) { | ||
| throw std::invalid_argument(fmt::format( | ||
| "Invalid arguments for constructing a non-empty bounded offset " | ||
| "interval: min({}) <= max({})", | ||
| min, | ||
| max)); | ||
| } | ||
|
|
||
| return {min, max}; | ||
| } | ||
|
Comment on lines
+26
to
+42
There was a problem hiding this comment. nit: maybe consider There was a problem hiding this comment. I'll continue iterating on this if we are to adopt it in more places. I intentionally named them unchecked/checked so that the name communicates to the caller that there is a "cost" involved. I'm still not sure whether the checked/unchecked are enough as constructor variants or whether they should be 2 separate structs and "propagate" the checked/uncheck property as a data structure "attribute". I.e. that all operations on that struct should be checked. Deferring this for now hoping that it will become clear when additional use-cases appear. |
||
|
|
||
| inline bool overlaps(const bounded_offset_interval& other) const noexcept { | ||
| return _min <= other._max && _max >= other._min; | ||
| } | ||
|
|
||
| inline bool contains(model::offset o) const noexcept { | ||
|
Comment on lines
+44
to
+48
There was a problem hiding this comment. inline is unnecessary here. it's implied when the definition is defined in the class like this |
||
| return _min <= o && o <= _max; | ||
| } | ||
|
|
||
| friend std::ostream& | ||
| operator<<(std::ostream& o, const bounded_offset_interval& r) { | ||
| fmt::print(o, "{{min: {}, max: {}}}", r._min, r._max); | ||
| return o; | ||
| } | ||
|
|
||
| inline model::offset min() const noexcept { return _min; } | ||
| inline model::offset max() const noexcept { return _max; } | ||
|
|
||
| private: | ||
| bounded_offset_interval(model::offset min, model::offset max) noexcept | ||
| : _min(min) | ||
| , _max(max) { | ||
| #ifndef NDEBUG | ||
|
There was a problem hiding this comment. is it impactful enough to performance to disable the check? There was a problem hiding this comment. My intention is to use this in hot-loops and I made the assumption that it will be impactful enough. E.g. I want checks like the one in or redpanda/src/v/storage/log_reader.cc Line 558 in 76a1ea2 to be compiled to nothing more than a single if statement that one would write manually. The redpanda/src/v/model/offset_interval.h Lines 32 to 42 in f13bfa6 |
||
| vassert( | ||
| min >= model::offset(0), "Offset interval min({}) must be >= 0", min); | ||
| vassert( | ||
| min <= max, | ||
| "Offset interval invariant not satisfied: min({}) <= max({})", | ||
| min, | ||
| max); | ||
| #endif | ||
| } | ||
|
|
||
| model::offset _min; | ||
| model::offset _max; | ||
| }; | ||
|
|
||
| } // namespace model | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there's a good place to unit test this bug, given it was masked by higher level code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will think about it. I'm proposing to remove the masking code in the next PR.