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/12/23 01:23:02 UTC
[GitHub] [pulsar-client-cpp] shibd opened a new pull request, #154: [feat] Support partitioned topic reader.
shibd opened a new pull request, #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154
### Motivation
In the current implementation, Reader does not yet support partitioned topics.
### Modifications
- ConsumerConfig support set `subscriptionMode`.
- MultiTopicConsumer support `hasMessageAvailable` and `acknowledgeCumulativeAsync` method.
- Reader support partitioned topic.
### Verifying this change
- Add all tests in `ReaderTest` to multi-partition topic scenarios.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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-client-cpp] shibd commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1068899826
##########
include/pulsar/ConsumerConfiguration.h:
##########
@@ -383,6 +383,26 @@ class PULSAR_PUBLIC ConsumerConfiguration {
*/
InitialPosition getSubscriptionInitialPosition() const;
+ /**
+ * Selects the subscription mode to be used when subscribing to a topic.
+ *
+ * <p>Options are:
+ * <ul>
+ * <li>{@link SubscriptionMode#Durable} (Default)</li>
+ * <li>{@link SubscriptionMode#NonDurable}</li>
+ * </ul>
+ *
+ * @param subscriptionMode the subscription mode value
+ */
+ ConsumerConfiguration& setSubscriptionMode(SubscriptionMode subscriptionMode);
+
+ /**
+ * Get subscription mode.
+ *
+ * @return
+ */
+ SubscriptionMode getSubscriptionMode() const;
Review Comment:
done.
--
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-client-cpp] BewareMyPower commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1062317982
##########
include/pulsar/ConsumerConfiguration.h:
##########
@@ -383,6 +383,26 @@ class PULSAR_PUBLIC ConsumerConfiguration {
*/
InitialPosition getSubscriptionInitialPosition() const;
+ /**
+ * Selects the subscription mode to be used when subscribing to a topic.
+ *
+ * <p>Options are:
+ * <ul>
+ * <li>{@link SubscriptionMode#Durable} (Default)</li>
+ * <li>{@link SubscriptionMode#NonDurable}</li>
+ * </ul>
+ *
+ * @param subscriptionMode the subscription mode value
+ */
+ ConsumerConfiguration& setSubscriptionMode(SubscriptionMode subscriptionMode);
+
+ /**
+ * Get subscription mode.
+ *
+ * @return
+ */
+ SubscriptionMode getSubscriptionMode() const;
Review Comment:
It's better not support setting the subscription mode. /cc @codelipenghui
--
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-client-cpp] BewareMyPower commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1072023978
##########
tests/ReaderTest.cc:
##########
@@ -36,10 +36,69 @@ using namespace pulsar;
static std::string serviceUrl = "pulsar://localhost:6650";
static const std::string adminUrl = "http://localhost:8080/";
-TEST(ReaderTest, testSimpleReader) {
+TEST(ReaderTest, testPartitionIndex) {
Review Comment:
Why did you move the `testPartitionIndex` here? Please avoid making these unrelated changes.
##########
tests/ReaderTest.cc:
##########
@@ -606,3 +653,5 @@ TEST(ReaderTest, testReceiveAfterSeek) {
client.close();
}
+
+INSTANTIATE_TEST_CASE_P(Pulsar, ReaderTest, ::testing::Values(true, false));
Review Comment:
```suggestion
INSTANTIATE_TEST_SUITE_P(Pulsar, ReaderTest, ::testing::Values(true, false));
```
`INSTANTIATE_TEST_CASE_P` is deprecated.
##########
lib/ReaderImpl.h:
##########
@@ -58,8 +58,9 @@ extern PULSAR_PUBLIC ConsumerConfiguration consumerConfigOfReader;
class PULSAR_PUBLIC ReaderImpl : public std::enable_shared_from_this<ReaderImpl> {
public:
- ReaderImpl(const ClientImplPtr client, const std::string& topic, const ReaderConfiguration& conf,
- const ExecutorServicePtr listenerExecutor, ReaderCallback readerCreatedCallback);
+ ReaderImpl(const ClientImplPtr client, const std::string& topic, const int partitions,
Review Comment:
Don't use a const value as a parameter. See a similar comment here: https://github.com/apache/pulsar-client-cpp/pull/169#discussion_r1065684370
--
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-client-cpp] shibd commented on pull request #154: [feat] Support partitioned topic reader.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#issuecomment-1418699864
@BewareMyPower @RobertIndie PTAL, Because it blocked TableView 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
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1055983371
##########
include/pulsar/ConsumerConfiguration.h:
##########
@@ -383,6 +383,26 @@ class PULSAR_PUBLIC ConsumerConfiguration {
*/
InitialPosition getSubscriptionInitialPosition() const;
+ /**
+ * Selects the subscription mode to be used when subscribing to a topic.
+ *
+ * <p>Options are:
+ * <ul>
+ * <li>{@link SubscriptionMode#Durable} (Default)</li>
+ * <li>{@link SubscriptionMode#NonDurable}</li>
+ * </ul>
+ *
+ * @param subscriptionMode the subscription mode value
+ */
+ void setSubscriptionMode(SubscriptionMode subscriptionMode);
Review Comment:
```suggestion
ConsumerConfiguration& setSubscriptionMode(SubscriptionMode subscriptionMode);
```
Support method chaining.
--
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-client-cpp] shibd commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1057132370
##########
include/pulsar/ConsumerConfiguration.h:
##########
@@ -383,6 +383,26 @@ class PULSAR_PUBLIC ConsumerConfiguration {
*/
InitialPosition getSubscriptionInitialPosition() const;
+ /**
+ * Selects the subscription mode to be used when subscribing to a topic.
+ *
+ * <p>Options are:
+ * <ul>
+ * <li>{@link SubscriptionMode#Durable} (Default)</li>
+ * <li>{@link SubscriptionMode#NonDurable}</li>
+ * </ul>
+ *
+ * @param subscriptionMode the subscription mode value
+ */
+ void setSubscriptionMode(SubscriptionMode subscriptionMode);
Review Comment:
I noticed that the style was different. Some returned config&, and some did not. I will unify it.
--
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-client-cpp] BewareMyPower commented on pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#issuecomment-1377147938
@shibd Could you resolve the conflicts and address the comments?
--
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-client-cpp] BewareMyPower commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1072008463
##########
lib/ClientConnection.h:
##########
@@ -65,6 +65,10 @@ class ConsumerImpl;
typedef std::shared_ptr<ConsumerImpl> ConsumerImplPtr;
typedef std::weak_ptr<ConsumerImpl> ConsumerImplWeakPtr;
+class ConsumerImplBase;
+typedef std::shared_ptr<ConsumerImplBase> ConsumerImplBasePtr;
+typedef std::weak_ptr<ConsumerImplBase> ConsumerImplBaseWeakPtr;
Review Comment:
These forward declarations are meaningless.
--
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-client-cpp] BewareMyPower commented on a diff in pull request #154: [feat] Support partitioned topic reader.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154#discussion_r1098099246
##########
lib/MultiTopicsConsumerImpl.h:
##########
@@ -53,10 +54,15 @@ class MultiTopicsConsumerImpl : public ConsumerImplBase {
public:
MultiTopicsConsumerImpl(ClientImplPtr client, TopicNamePtr topicName, int numPartitions,
const std::string& subscriptionName, const ConsumerConfiguration& conf,
- LookupServicePtr lookupServicePtr);
+ LookupServicePtr lookupServicePtr,
+ const Commands::SubscriptionMode = Commands::SubscriptionModeDurable,
+ boost::optional<MessageId> startMessageId = boost::none);
+
MultiTopicsConsumerImpl(ClientImplPtr client, const std::vector<std::string>& topics,
const std::string& subscriptionName, TopicNamePtr topicName,
- const ConsumerConfiguration& conf, LookupServicePtr lookupServicePtr_);
+ const ConsumerConfiguration& conf, LookupServicePtr lookupServicePtr_,
+ const Commands::SubscriptionMode = Commands::SubscriptionModeDurable,
Review Comment:
```suggestion
Commands::SubscriptionMode = Commands::SubscriptionModeDurable,
```
##########
lib/MultiTopicsConsumerImpl.h:
##########
@@ -53,10 +54,15 @@ class MultiTopicsConsumerImpl : public ConsumerImplBase {
public:
MultiTopicsConsumerImpl(ClientImplPtr client, TopicNamePtr topicName, int numPartitions,
const std::string& subscriptionName, const ConsumerConfiguration& conf,
- LookupServicePtr lookupServicePtr);
+ LookupServicePtr lookupServicePtr,
+ const Commands::SubscriptionMode = Commands::SubscriptionModeDurable,
Review Comment:
```suggestion
Commands::SubscriptionMode = Commands::SubscriptionModeDurable,
```
--
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-client-cpp] BewareMyPower merged pull request #154: [feat] Support partitioned topic reader.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #154:
URL: https://github.com/apache/pulsar-client-cpp/pull/154
--
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