[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16969: [refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead

[GitBox <gi...@apache.org>]

BewareMyPower commented on code in PR #16969:
URL: https://github.com/apache/pulsar/pull/16969#discussion_r950047034


##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -52,6 +53,24 @@ MultiTopicsConsumerImpl::MultiTopicsConsumerImpl(ClientImplPtr client, const std
     } else {
         unAckedMessageTrackerPtr_.reset(new UnAckedMessageTrackerDisabled());
     }
+    auto partitionsUpdateInterval = static_cast<unsigned int>(client_->conf().getPartitionsUpdateInterval());
+    if (partitionsUpdateInterval > 0) {
+        listenerExecutor_ = client_->getListenerExecutorProvider()->get();
+        partitionsUpdateTimer_ = listenerExecutor_->createDeadlineTimer();
+        partitionsUpdateInterval_ = boost::posix_time::seconds(partitionsUpdateInterval);
+        lookupServicePtr_ = client_->getLookup();
+    }
+}
+
+ConsumerImplBasePtr MultiTopicsConsumerImpl::createPartitionedConsumer(
+    ClientImplPtr client, const std::string singleTopic, const int numPartitions,
+    const std::string& subscriptionName, TopicNamePtr topicName, const ConsumerConfiguration& conf,
+    const LookupServicePtr lookupServicePtr) {
+    MultiTopicsConsumerImpl* partitionedConsumer = new MultiTopicsConsumerImpl(
+        client, {singleTopic}, subscriptionName, topicName, conf, lookupServicePtr);
+    partitionedConsumer->topicsPartitions_.emplace(singleTopic, numPartitions);
+    std::shared_ptr<MultiTopicsConsumerImpl> partitionedConsumerPtr(partitionedConsumer);
+    return partitionedConsumerPtr;
 }

Review Comment:
   Adding a static method that returns a shared pointer of `MultiTopicsConsumerImpl` here looks weird and inconsistent. I found it's suggested by the comment here: https://github.com/apache/pulsar/pull/16969#discussion_r939897369.
   
   However, we can add another overload constructor like:
   
   ```c++
       MultiTopicsConsumerImpl(ClientImplPtr client, const std::string& singleTopic, const int numPartitions,
                               const std::string& subscriptionName, TopicNamePtr topicName,
                               const ConsumerConfiguration& conf, const LookupServicePtr lookupServicePtr)
           : MultiTopicsConsumerImpl(client, {singleTopic}, subscriptionName, topicName, conf,
                                     lookupServicePtr) {
           topicsPartitions_[singleTopic] = numPartitions;
       }
   ```
   
   It's more simple than the current code. /cc @RobertIndie 
   
   BTW, since the implementation of the overload constructor is very simple (only 1 line except calling another constructor), we can write it in the header file directly.



-- 
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