Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

model: make kafka::next_offset saturate rather than overflow #18508

Merged

Conversation

nvartolomei
Copy link
Contributor

This is similar to change introduced in
8c3777b but this time for kafka::offset.

Quoting from the previous commit message:

model::offset::max() is often used to indicate "no upper bound" on
operations. E.g. for tiered storage uploads[^1], for reading from local
storage[^2], etc.

We also do often convert from closed to opened offset intervals
representations. E.g. committed offset to LSO and the other way around.

When combined, these can result in unexpected behaviors. In particular,
if on a read path the max offset is specified as model::offset_max() but
at lower level this is converted into an exclusive offset by calling
next_offset(model::offset::max()), the result is model::offset::min()
aka -2^63.

This is dangerous. Let's instead saturate the offset similar to how we
saturate prev_offset.

We also have a few cases where we just do `o + model::offset(1)`. These
should be refactored to use next_offset too.

This isn't fixing any existing known bug. Discovered this while trying
to rewrite some logic related to tiered storage uploads.

[^1]: https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/archival/ntp_archiver_service.cc#L1656
[^2]: https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/cluster/migrations/tx_manager_migrator.cc#L219

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@nvartolomei nvartolomei force-pushed the nv/saturating-kafka-next-offset branch from 5aa1cd9 to 6c04aa2 Compare May 22, 2024 13:28
This is similar to change introduced in
8c3777b but this time for
kafka::offset.

Quoting from the previous commit message:

    model::offset::max() is often used to indicate "no upper bound" on
    operations. E.g. for tiered storage uploads[^1], for reading from local
    storage[^2], etc.

    We also do often convert from closed to opened offset intervals
    representations. E.g. committed offset to LSO and the other way around.

    When combined, these can result in unexpected behaviors. In particular,
    if on a read path the max offset is specified as model::offset_max() but
    at lower level this is converted into an exclusive offset by calling
    next_offset(model::offset::max()), the result is model::offset::min()
    aka -2^63.

    This is dangerous. Let's instead saturate the offset similar to how we
    saturate prev_offset.

    We also have a few cases where we just do `o + model::offset(1)`. These
    should be refactored to use next_offset too.

    This isn't fixing any existing known bug. Discovered this while trying
    to rewrite some logic related to tiered storage uploads.

    [^1]: https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/archival/ntp_archiver_service.cc#L1656
    [^2]: https://github.com/redpanda-data/redpanda/blob/79bf7eed6e04da1d0987b5abd719c4b289dde761/src/v/cluster/migrations/tx_manager_migrator.cc#L219
@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/49428#018fa0ba-65ab-4dcc-accd-8df9b56810a2:

"rptest.tests.availability_test.AvailabilityTests.test_availability_when_one_node_failed"

@nvartolomei
Copy link
Contributor Author

/ci-repeat 1
skip-redpanda-build
tests/rptest/tests/availability_test.py

@nvartolomei nvartolomei merged commit ff71a74 into redpanda-data:dev May 24, 2024
18 checks passed
@nvartolomei nvartolomei deleted the nv/saturating-kafka-next-offset branch May 24, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants