Skip to content

KAFKA-7535:KafkaConsumer doesn't report records-lag if isolation.level is read_committed - #5835

Closed
lambdaliu wants to merge 3 commits into
apache:trunkfrom
lambdaliu:KAFKA-7535
Closed

KAFKA-7535:KafkaConsumer doesn't report records-lag if isolation.level is read_committed#5835
lambdaliu wants to merge 3 commits into
apache:trunkfrom
lambdaliu:KAFKA-7535

Conversation

@lambdaliu

@lambdaliu lambdaliu commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

FetchResponse should return the partitionData's lastStabeleOffset

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@ijuma ijuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR. Can you please add a test? cc @dhruvilshah3

@ijuma

ijuma commented Oct 24, 2018

Copy link
Copy Markdown
Member

If you don't have time to write a test, let us know and we will help.

@lindong28

Copy link
Copy Markdown
Member

@lambdaliu Thanks for the fix! Currently the patch still uses FetchResponse.INVALID_LAST_STABLE_OFFSET as lastStableOffset when there is need to convert data. Should we change this as well?

@lambdaliu

Copy link
Copy Markdown
Contributor Author

@ijuma @lindong28 Thanks for your review. I added some tests to changed FetchResponse.INVALID_LAST_STABLE_OFFSET to lastStableOffset under date conversion, please review this PR again.

@lindong28 lindong28 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lambdaliu Thanks for the update. LGTM.

@lindong28

Copy link
Copy Markdown
Member

@dhruvilshah3 Would you like to review this PR?

@dhruvilshah3 dhruvilshah3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM

lindong28 pushed a commit that referenced this pull request Oct 25, 2018
…el is read_committed

FetchResponse should return the partitionData's lastStabeleOffset

Author: lambdaliu <lambdaliu@tencent.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Dhruvil Shah <dhruvil@confluent.io>, Dong Lin <lindong28@gmail.com>

Closes #5835 from lambdaliu/KAFKA-7535

(cherry picked from commit 73da591)
Signed-off-by: Dong Lin <lindong28@gmail.com>
@lindong28 lindong28 closed this in 73da591 Oct 25, 2018
@ijuma

ijuma commented Oct 25, 2018

Copy link
Copy Markdown
Member

@lindong28 can we cherry-pick this to 2.0 as well? That's when the regression was introduced.

lindong28 pushed a commit that referenced this pull request Oct 25, 2018
…el is read_committed

FetchResponse should return the partitionData's lastStabeleOffset

Author: lambdaliu <lambdaliu@tencent.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Dhruvil Shah <dhruvil@confluent.io>, Dong Lin <lindong28@gmail.com>

Closes #5835 from lambdaliu/KAFKA-7535
@lindong28

Copy link
Copy Markdown
Member

Thanks for the patch. Merged to 2.0, 2.1 and trunk.

@ijuma The PR could not merge cleanly into 2.0. I was manually modifying the patch :)

@ijuma

ijuma commented Oct 25, 2018

Copy link
Copy Markdown
Member

Perfect, thanks @lindong28 and @lambdaliu.

@lambdaliu
lambdaliu deleted the KAFKA-7535 branch January 4, 2019 11:20
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…el is read_committed

FetchResponse should return the partitionData's lastStabeleOffset

Author: lambdaliu <lambdaliu@tencent.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Dhruvil Shah <dhruvil@confluent.io>, Dong Lin <lindong28@gmail.com>

Closes apache#5835 from lambdaliu/KAFKA-7535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants