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

[#35] Use LitePullConsumer model instead of default pull consumer #46

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

yiduwangkai
Copy link
Contributor

@yiduwangkai yiduwangkai commented Jul 24, 2022

close #35
Use DefaultLitePullConsumer model instead of default pull consumer.

Tips
Thank you very much for contributing to Apache rocketmq-flink.
What is the purpose of the pull request
*solve some code about using defaultPullConsumer api

Brief change log
(for example:)

*Modify
RocketMQConfig.java
RocketMQSourceFunction.java
RocketMQSourceEnumerator.java
RocketMQPartitionSplitReader.java
RocketMQSourceTest.java

@yiduwangkai
Copy link
Contributor Author

there are some problems about this code, like home to Use defaultlitPullConsumer to get offset and perform offset self-management

@ShannonDing ShannonDing changed the title [upgrade consumer api]Use litePullConsumer model instead of default p… [ISSUE #35]Use litePullConsumer model instead of default p… Aug 15, 2022
@ShannonDing ShannonDing added the enhancement New feature or request label Aug 15, 2022
Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

Thanks for @yiduwangkai contribution. I have left the comments for this pull request. Did you test the changes? In addtion, please resolve the conflicts.

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2022

Codecov Report

Attention: Patch coverage is 5.15464% with 92 lines in your changes missing coverage. Please review.

Project coverage is 28.62%. Comparing base (90b00be) to head (3f5fbe4).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
.../rocketmq/flink/legacy/RocketMQSourceFunction.java 0.00% 57 Missing ⚠️
...nk/source/reader/RocketMQPartitionSplitReader.java 0.00% 25 Missing ⚠️
...nk/source/enumerator/RocketMQSourceEnumerator.java 0.00% 9 Missing ⚠️
...g/apache/rocketmq/flink/source/RocketMQSource.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #46      +/-   ##
============================================
+ Coverage     28.50%   28.62%   +0.11%     
  Complexity      164      164              
============================================
  Files            62       62              
  Lines          2529     2536       +7     
  Branches        269      269              
============================================
+ Hits            721      726       +5     
- Misses         1741     1743       +2     
  Partials         67       67              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SteNicholas
Copy link
Member

SteNicholas commented Aug 30, 2022

Thanks for @yiduwangkai updates. Could you please resolve the conflicts and squash the commits?

@yiduwangkai yiduwangkai force-pushed the upgrade_consumer_api branch 2 times, most recently from 7b39d52 to f1861be Compare September 4, 2022 17:00
Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

Thanks for @yiduwangkai updates. I left some comments for this update. PTAL.

@SteNicholas SteNicholas changed the title [ISSUE #35]Use litePullConsumer model instead of default p… [#35] Use LitePullConsumer model instead of default pull consumer Oct 9, 2022
@zhouxinyu
Copy link
Member

Thanks, @yiduwangkai for your contributions. I left some comments, but I'm not familiar with rocketmq-flink, so please @SteNicholas help have a look.

@zhouxinyu
Copy link
Member

Since not all broker versions support lite pull, we need to clarify it in our doc.

Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use litePullConsumer model instead of default pull consumer.
6 participants