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.

Sign up for GitHub

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

Jump to bottom

[feat] Support partitioned topic reader. #154

Merged
merged 2 commits into from
Feb 7, 2023
Merged

[feat] Support partitioned topic reader. #154

merged 2 commits into from
Feb 7, 2023

Conversation

shibd
Copy link
Member

@shibd shibd commented Dec 23, 2022 •
edited
Loading

Motivation

In the current implementation, Reader does not yet support partitioned topics.

Modifications

  • MultiTopicConsumer support hasMessageAvailable and acknowledgeCumulativeAsync method.
  • Reader support partitioned topic.

Verifying this change

  • Add all tests in ReaderTest to multi-partition topic scenarios.
  • Add ConsumerTest.testAcknowledgeCumulativeWithPartition unit test to cover MultiTopicConsumer.acknowledgeCumulativeAsync method.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@shibd shibd mentioned this pull request Dec 29, 2022
Closed
4 tasks
@BewareMyPower
Copy link
Contributor

@shibd Could you resolve the conflicts and address the comments?

@shibd shibd requested a review from BewareMyPower January 13, 2023 03:55
BewareMyPower
BewareMyPower reviewed Jan 17, 2023
View reviewed changes
lib/ReaderImpl.h Outdated Show resolved Hide resolved
tests/ReaderTest.cc Outdated Show resolved Hide resolved
tests/ReaderTest.cc Outdated Show resolved Hide resolved
@shibd shibd requested a review from BewareMyPower January 18, 2023 01:51
@shibd shibd self-assigned this Jan 19, 2023
@shibd shibd added the enhancement New feature or request label Jan 19, 2023
@shibd shibd added this to the 3.2.0 milestone Jan 19, 2023
@shibd shibd requested a review from RobertIndie January 19, 2023 09:12
@shibd
Copy link
Member Author

shibd commented Feb 6, 2023

@BewareMyPower @RobertIndie PTAL, Because it blocked TableView pr.

BewareMyPower
BewareMyPower reviewed Feb 7, 2023
View reviewed changes
lib/MultiTopicsConsumerImpl.h Outdated Show resolved Hide resolved
lib/MultiTopicsConsumerImpl.h Outdated Show resolved Hide resolved
BewareMyPower
BewareMyPower approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just need to remove some const qualifiers on value parameters.

@BewareMyPower BewareMyPower merged commit 70ccd83 into apache:main Feb 7, 2023
@XXXMrG XXXMrG mentioned this pull request Apr 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@RobertIndie RobertIndie Awaiting requested review from RobertIndie

Assignees

@shibd shibd

Labels
enhancement New feature or request
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@shibd @BewareMyPower