Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

fix: ListOffset interface returns an incorrect result #1747

Open
wants to merge 1 commit into
base: branch-2.9.2.24
Choose a base branch
from

Conversation

keyboardbobo
Copy link
Contributor

The commit offset and partition data expire at the same time, and the ListOffset interface returns an incorrect result

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #1737

Motivation

The commit offset and partition data expire at the same time, and the ListOffset interface returns an incorrect result

Modifications

If all the data expires, the entry of lac becomes -1, and the actual offset will be 1 smaller than LATEST, add 1 back

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

The commit offset and partition data expire at the same time, and the ListOffset interface returns an incorrect result
@CLAassistant
Copy link

CLAassistant commented Mar 1, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting finding.
I am not sure that we can build a test.

How did you test that this fix actually fixed the problem ?

@keyboardbobo
Copy link
Contributor Author

This is an interesting finding. I am not sure that we can build a test.

How did you test that this fix actually fixed the problem ?

@eolivelli
This is indeed difficult to test, but the problem does exist. I tested the following scenarios, and the producer and consumer offsets all need to meet expectations:

  1. __consumer_offset data has not expired, topic data has not expired
  2. __consumer_offset data is expired, topic data is not expired
  3. __consumer_offset data expires, topic data expires
    At present, only the consumer offset queried in the third case is wrong, The third case corresponds to if (position.compareTo(lac) > 0). I don't know if you have a better test method?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants