You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/04 07:25:49 UTC
[GitHub] [pulsar] Lannnnh opened a new pull request, #15020: [client-cpp] add subscription properties to consumer for cpp
Lannnnh opened a new pull request, #15020:
URL: https://github.com/apache/pulsar/pull/15020
# Motivation
Pulsar already support entry filter, But pulsar-client-cpp ConsumerImpl not contains subscriptionProperties Option. So, this PR want to solve it.
# Changes
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1092539618
@BewareMyPower Hello, this PR has been suspended for a few days. Next, how to proceed further?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1092547680
It's blocked by the failed CI caused by some flaky tests. I reran the failed CI again just now.
<img width="885" alt="image" src="https://user-images.githubusercontent.com/18204803/162387170-61911439-6c79-4441-b2d5-6426474abbfc.png">
BTW, you can also comment `/pulsarbot run-failure-checks` to rerun the tests. For example, https://github.com/apache/pulsar/pull/15081#issuecomment-1092507443
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1088208309
@Lannnnh:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1087208981
@Lannnnh:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #15020:
URL: https://github.com/apache/pulsar/pull/15020
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1092548868
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r843424346
##########
pulsar-client-cpp/lib/ConsumerConfiguration.cc:
##########
@@ -214,6 +214,19 @@ ConsumerConfiguration& ConsumerConfiguration::setProperties(
return *this;
}
+std::map<std::string, std::string>& ConsumerConfiguration::getSubscriptionProperties() const {
+ return impl_->subscriptionProperties;
+}
+
+ConsumerConfiguration& ConsumerConfiguration::setSubscriptionProperties(
+ const std::map<std::string, std::string>& subscriptionProperties) {
+ for (std::map<std::string, std::string>::const_iterator it = subscriptionProperties.begin();
+ it != subscriptionProperties.end(); it++) {
+ impl_->subscriptionProperties.insert(std::make_pair(it->first, it->second));
+ }
+ return *this;
Review Comment:
It has been revised as the previous question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1087219464
@Lannnnh:Thanks for providing doc info!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r843423094
##########
pulsar-client-cpp/lib/Commands.cc:
##########
@@ -308,6 +309,14 @@ SharedBuffer Commands::newSubscribe(const std::string& topic, const std::string&
subscribe->mutable_metadata()->AddAllocated(keyValue);
}
+ for (std::map<std::string, std::string>::const_iterator it = subscriptionProperties.begin();
+ it != subscriptionProperties.end(); it++) {
Review Comment:
thanks for your suggestion, it has been revised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1089739973
@BewareMyPower ok, I have been added a test in ConsumerConfigurationTest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1089737811
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh closed pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh closed pull request #15020: [client-cpp] add subscription properties to consumer for cpp
URL: https://github.com/apache/pulsar/pull/15020
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1088208323
@Lannnnh:Thanks for providing doc info!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r842839160
##########
pulsar-client-cpp/lib/Commands.cc:
##########
@@ -308,6 +309,14 @@ SharedBuffer Commands::newSubscribe(const std::string& topic, const std::string&
subscribe->mutable_metadata()->AddAllocated(keyValue);
}
+ for (std::map<std::string, std::string>::const_iterator it = subscriptionProperties.begin();
+ it != subscriptionProperties.end(); it++) {
Review Comment:
I think we can use the C++11 style for new code. Though some legacy code still uses older style. We can do some refactoring to these legacy code later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r842840849
##########
pulsar-client-cpp/lib/ConsumerConfiguration.cc:
##########
@@ -214,6 +214,19 @@ ConsumerConfiguration& ConsumerConfiguration::setProperties(
return *this;
}
+std::map<std::string, std::string>& ConsumerConfiguration::getSubscriptionProperties() const {
+ return impl_->subscriptionProperties;
+}
+
+ConsumerConfiguration& ConsumerConfiguration::setSubscriptionProperties(
+ const std::map<std::string, std::string>& subscriptionProperties) {
+ for (std::map<std::string, std::string>::const_iterator it = subscriptionProperties.begin();
+ it != subscriptionProperties.end(); it++) {
+ impl_->subscriptionProperties.insert(std::make_pair(it->first, it->second));
+ }
+ return *this;
Review Comment:
It's similar to my previous comment. For new code, use range-based for loop and the `emplace` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r843470072
##########
pulsar-client-cpp/lib/ConsumerConfiguration.cc:
##########
@@ -214,6 +214,19 @@ ConsumerConfiguration& ConsumerConfiguration::setProperties(
return *this;
}
+std::map<std::string, std::string>& ConsumerConfiguration::getSubscriptionProperties() const {
+ return impl_->subscriptionProperties;
+}
+
+ConsumerConfiguration& ConsumerConfiguration::setSubscriptionProperties(
+ const std::map<std::string, std::string>& subscriptionProperties) {
+ for (const auto& subscriptionProperty : subscriptionProperties) {
+ impl_->subscriptionProperties.emplace(
+ std::make_pair(subscriptionProperty.first, subscriptionProperty.second));
Review Comment:
ok, it has been revised, thanks for your attention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#discussion_r843425729
##########
pulsar-client-cpp/lib/ConsumerConfiguration.cc:
##########
@@ -214,6 +214,19 @@ ConsumerConfiguration& ConsumerConfiguration::setProperties(
return *this;
}
+std::map<std::string, std::string>& ConsumerConfiguration::getSubscriptionProperties() const {
+ return impl_->subscriptionProperties;
+}
+
+ConsumerConfiguration& ConsumerConfiguration::setSubscriptionProperties(
+ const std::map<std::string, std::string>& subscriptionProperties) {
+ for (const auto& subscriptionProperty : subscriptionProperties) {
+ impl_->subscriptionProperties.emplace(
+ std::make_pair(subscriptionProperty.first, subscriptionProperty.second));
Review Comment:
There is no need to emplace a `std::pair`. See following example:
```c++
std::map<std::string, std::string> m;
m.emplace("key", "value");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Lannnnh commented on pull request #15020: [client-cpp] add subscription properties to consumer for cpp
Posted by GitBox <gi...@apache.org>.
Lannnnh commented on PR #15020:
URL: https://github.com/apache/pulsar/pull/15020#issuecomment-1092776085
@BewareMyPower Thanks for the reminder, now that all tests have passed. Next, need your review/accept to merge this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org