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

[client-cpp] add subscription properties to consumer for cpp #15020

Merged
merged 5 commits into from
Apr 8, 2022
Merged

[client-cpp] add subscription properties to consumer for cpp #15020

merged 5 commits into from
Apr 8, 2022

Conversation

Lannnnh
Copy link
Contributor

@Lannnnh Lannnnh commented Apr 4, 2022 •
edited
Loading

Motivation

Pulsar already support entry filter, But pulsar-client-cpp ConsumerImpl not contains subscriptionProperties Option. So, this PR want to solve it.

Modifications

Enable subscriptionProperties on consumer for cpp client.

Results

subscriptionProperties are added for CommandSubscribe. However, there is no way to verify the consumer subscriptionProperties. so I didn't add any specific tests.

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • no-need-doc

Why?
this PR only involves pulsar-client-cpp consumer subscribe opition modification

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

@Lannnnh:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

@Lannnnh:Thanks for providing doc info!

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 4, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

@Lannnnh:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs doc-label-missing and removed doc-not-needed Your PR changes do not impact docs doc-label-missing labels Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

@Lannnnh:Thanks for providing doc info!

@BewareMyPower BewareMyPower added type/feature The PR added a new feature or issue requested a new feature component/client-c++ labels Apr 5, 2022
@BewareMyPower BewareMyPower added this to the 2.11.0 milestone Apr 5, 2022
BewareMyPower
BewareMyPower reviewed Apr 5, 2022
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.

Please add a test in ConsumerConfigurationTest.

See #12076 for example.

@Lannnnh Lannnnh force-pushed the subscribe_subscription_propertied_for_cpp branch from 4b85cec to dae3229 Compare April 6, 2022 03:15
@Lannnnh Lannnnh closed this Apr 6, 2022
@Lannnnh Lannnnh reopened this Apr 6, 2022
@Lannnnh
Copy link
Contributor Author

Lannnnh commented Apr 6, 2022

@BewareMyPower ok, I have been added a test in ConsumerConfigurationTest.

@Lannnnh Lannnnh force-pushed the subscribe_subscription_propertied_for_cpp branch from f865596 to 82878b4 Compare April 6, 2022 11:18
@Lannnnh Lannnnh requested a review from BewareMyPower April 6, 2022 14:38
@Lannnnh
Copy link
Contributor Author

Lannnnh commented Apr 8, 2022

@BewareMyPower Hello, this PR has been suspended for a few days. Next, how to proceed further?

@BewareMyPower
Copy link
Contributor

It's blocked by the failed CI caused by some flaky tests. I reran the failed CI again just now.

image

BTW, you can also comment /pulsarbot run-failure-checks to rerun the tests. For example, #15081 (comment)

@BewareMyPower
Copy link
Contributor

If it still failed, you can try to rebase to master and push by force again because the latest commits might fix some flaky tests.

@Lannnnh Lannnnh force-pushed the subscribe_subscription_propertied_for_cpp branch from 40b0ca3 to 21070fe Compare April 8, 2022 07:51
@Lannnnh
Copy link
Contributor Author

Lannnnh commented Apr 8, 2022

@BewareMyPower Thanks for the reminder, now that all tests have passed. Next, need your review/accept to merge this PR.

@BewareMyPower BewareMyPower merged commit fc1cf25 into apache:master Apr 8, 2022
@Lannnnh Lannnnh deleted the subscribe_subscription_propertied_for_cpp branch April 8, 2022 14:47
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Apr 18, 2022
@Lannnnh @aparajita-singh-flipkart
[client-cpp] add subscription properties to consumer for cpp (apache#…
41dcd8e 
…15020)

# Motivation
Pulsar already support entry filter, But pulsar-client-cpp ConsumerImpl not contains subscriptionProperties Option. So, this PR want to solve it.

# Modifications
Enable subscriptionProperties on consumer for cpp client.
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@Lannnnh
[client-cpp] add subscription properties to consumer for cpp (apache#…
fd680fc 
…15020)

# Motivation
Pulsar already support entry filter, But pulsar-client-cpp ConsumerImpl not contains subscriptionProperties Option. So, this PR want to solve it.

# Modifications
Enable subscriptionProperties on consumer for cpp client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

Assignees

@Lannnnh Lannnnh

Labels
doc-not-needed Your PR changes do not impact docs type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
2.11.0
Development

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