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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
