kafka: return lowest offset if requested epoch is lower than local log #9264
Conversation
| const auto first_local_term = _partition->get_term( | ||
| _partition->start_offset()); | ||
| // found in local state | ||
| const auto first_local_offset = _partition->start_offset(); |
There was a problem hiding this comment.
we should post the link to the code of the java upstream here
| // Return the offset of the next-highest term, i.e. the start of the local | ||
| // log. This mirrors behavior in Kafka, see | ||
| // https://github.com/apache/kafka/pull/5678 for more details. | ||
| return _translator->from_log_offset(first_local_offset); |
There was a problem hiding this comment.
this is great. how do we write a test that exercises this path.
There was a problem hiding this comment.
I updated one of the tests added when implementing KIP-320. Will look into testing more organically
There was a problem hiding this comment.
Opted to add a unit test, per @graphcareful's suggestion
|
Let's get this going, backport and shoot for a minor next week. |
|
This seems like the correct new behavior, but also a significant change from what existing apps had been depending on, so definitely needs release notes or changes to any doc page mentioning how this works. @micheleRP. I'm glad it happens to be coming near the first 23.1 release @piyushredpanda should we also mention this in older version docs in our Kafka compat page (basically it's an incompatibility we're correcting in 23.1) |
andrwng
force-pushed
the
get-leader-epoch-last-offset
branch
from
d08a5a8 to
d6f3889
Compare
Lean towards no. I don't know if many users and applications have noticed this; this behavior incompat has certainly been around years and has only just bubbled up. (Sidenote, we will be backporting fix to older branches, not just in v23.1.x) |
andrwng
force-pushed
the
get-leader-epoch-last-offset
branch
from
d6f3889 to
53830d8
Compare
kafka: return lowest offset if requested epoch is lower than local log If reading from tiered storage is disabled, we would previously return an offset -1 if the epoch queried fell below the start of our local log. This doesn't match what happens in Kafka, which returns the next available offset in a higher epoch. This behavior is matched by our lookup when reading from cloud storage. This patch updates the Kafka handler to return the start offset of our local log if the requested term falls below the local log's beginning.
andrwng
force-pushed
the
get-leader-epoch-last-offset
branch
from
53830d8 to
350c8ec
Compare
|
/backport v23.1.x |
|
/backport v22.3.x |
|
/backport v22.2.x |
|
Failed to run cherry-pick command. I executed the below command: |
|
Failed to run cherry-pick command. I executed the below command: |
|
Thank you 🙏🏾 🚀 |
If reading from tiered storage is disabled, we would previously return an offset -1 if the epoch queried fell below the start of our local log.
This doesn't match what happens in Kafka, which returns the next available offset in a higher epoch. This behavior is matched by our lookup when reading from cloud storage.
This patch updates the Kafka handler to return the start offset of our local log if the requested term falls below the local log's beginning.
Backports Required
Release Notes
Bug Fixes