You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Shoothzj (via GitHub)" <gi...@apache.org> on 2023/06/01 00:18:34 UTC
[GitHub] [pulsar-client-cpp] Shoothzj opened a new pull request, #277: [feat][Message] Add getIndex method on Message
Shoothzj opened a new pull request, #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277
### Motivation
After #276 , We already can consume messages contains index well. Now we can add a method allow user to getIndex.
Index is an optional brokerMetadata. If the index not exists, we will return -1
### Verifying this change
- Add the assertions to assert the index not equals -1.
### 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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1213380059
##########
include/pulsar/Message.h:
##########
@@ -124,6 +125,12 @@ class PULSAR_PUBLIC Message {
*/
void setMessageId(const MessageId& messageId) const;
+ /**
+ * Get the index of this message, if it doesn't exist, return -1
+ * @return
+ */
+ const int64_t getIndex() const;
Review Comment:
@BewareMyPower Fixed
--
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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1213380267
##########
lib/MessageImpl.cc:
##########
@@ -20,7 +20,8 @@
namespace pulsar {
-MessageImpl::MessageImpl() : metadata(), payload(), messageId(), cnx_(0), topicName_(), redeliveryCount_() {}
+MessageImpl::MessageImpl()
+ : metadata(), brokerEntryMetadata(), payload(), messageId(), cnx_(0), topicName_(), redeliveryCount_() {}
Review Comment:
@BewareMyPower removed
##########
lib/Message.cc:
##########
@@ -136,6 +140,15 @@ void Message::setMessageId(const MessageId& messageID) const {
return;
}
+const int64_t Message::getIndex() const {
+ if (!impl_) {
+ return -1;
+ } else {
+ // casting uint64_t to int64_t, server definition ensures that's safe
+ return static_cast<int64_t>(impl_->brokerEntryMetadata.index());
Review Comment:
@BewareMyPower Fixed
--
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] Shoothzj commented on pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#issuecomment-1579751638
ping @BewareMyPower
--
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 #277: [feat][Message] Add getIndex method on Message
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219183350
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -25,19 +25,58 @@ using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
Review Comment:
The only difference between these two tests is here. I think you can use `INSTANTIATE_TEST_SUITE_P` to reduce duplicated code. See `AcknowledgeTest.cc` for example.
But it's only an improvement. If you think it's too complicated, just resolve this comment.
--
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 #277: [feat][Message] Add getIndex method on Message
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219776507
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -20,24 +20,74 @@
#include <gtest/gtest.h>
#include <pulsar/Client.h>
+#include "lib/Latch.h"
+
using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
+ Result producerResult =
+ client.createProducer("persistent://public/default/topic-non-batch", producerConfiguration, producer);
ASSERT_EQ(producerResult, ResultOk);
Consumer consumer;
- Result consumerResult =
- client.subscribe("persistent://public/default/testConsumeSuccess", "testConsumeSuccess", consumer);
+ Result consumerResult = client.subscribe("persistent://public/default/topic-non-batch", "sub", consumer);
ASSERT_EQ(consumerResult, ResultOk);
- const auto msg = MessageBuilder().setContent("testConsumeSuccess").build();
- Result sendResult = producer.send(msg);
- ASSERT_EQ(sendResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
+ ASSERT_EQ(sendResult, ResultOk);
+ }
+
+ Message receivedMsg;
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ printf("receive index: %d\n", i);
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_EQ(receivedMsg.getIndex(), i);
+ Result ackResult = consumer.acknowledge(receivedMsg);
+ ASSERT_EQ(ackResult, ResultOk);
+ }
+ client.close();
+}
+
+TEST(BrokerMetadataTest, testConsumeBatchSuccess) {
+ Client client{"pulsar://localhost:6650"};
+ Producer producer;
+ Result producerResult = client.createProducer("persistent://public/default/topic-batch", producer);
+ ASSERT_EQ(producerResult, ResultOk);
+ Consumer consumer;
+ Result consumerResult = client.subscribe("persistent://public/default/topic-batch", "sub", consumer);
+ ASSERT_EQ(consumerResult, ResultOk);
+
+ Latch latch(10);
+ auto sendCallback = [&latch](Result result, const MessageId& id) {
+ ASSERT_EQ(result, ResultOk);
+ latch.countdown();
+ };
+
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ producer.sendAsync(msg, sendCallback);
+ }
+
+ latch.wait();
+
Message receivedMsg;
- Result receiveResult = consumer.receive(receivedMsg);
- ASSERT_EQ(receiveResult, ResultOk);
- ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess");
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_GE(receivedMsg.getIndex(), 0);
+ ASSERT_LT(receivedMsg.getIndex(), 10);
Review Comment:
Why did you check `0 < index < 10`? The `index` should be equal to `i`.
--
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 #277: [feat][Message] Add getIndex method on Message
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219186715
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -25,19 +25,58 @@ using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
+ Result producerResult =
+ client.createProducer("persistent://public/default/topic-non-batch", producerConfiguration, producer);
ASSERT_EQ(producerResult, ResultOk);
Consumer consumer;
- Result consumerResult =
- client.subscribe("persistent://public/default/testConsumeSuccess", "testConsumeSuccess", consumer);
+ Result consumerResult = client.subscribe("persistent://public/default/topic-non-batch", "sub", consumer);
ASSERT_EQ(consumerResult, ResultOk);
- const auto msg = MessageBuilder().setContent("testConsumeSuccess").build();
- Result sendResult = producer.send(msg);
- ASSERT_EQ(sendResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
+ ASSERT_EQ(sendResult, ResultOk);
+ }
+
+ Message receivedMsg;
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ printf("receive index: %d\n", i);
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_EQ(receivedMsg.getIndex(), i);
+ Result ackResult = consumer.acknowledge(receivedMsg);
+ ASSERT_EQ(ackResult, ResultOk);
+ }
+ client.close();
+}
+
+TEST(BrokerMetadataTest, testConsumeBatchSuccess) {
+ Client client{"pulsar://localhost:6650"};
+ Producer producer;
+ Result producerResult = client.createProducer("persistent://public/default/topic-batch", producer);
+ ASSERT_EQ(producerResult, ResultOk);
+ Consumer consumer;
+ Result consumerResult = client.subscribe("persistent://public/default/topic-batch", "sub", consumer);
+ ASSERT_EQ(consumerResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
Review Comment:
If you want to verify the batch send case, you should use `sendAsync` instead of `send` because batching is meaningless for synchronous send.
--
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] Shoothzj commented on pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#issuecomment-1571238351
@BewareMyPower PTAL
--
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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219587699
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -25,19 +25,58 @@ using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
Review Comment:
@BewareMyPower I am fine with this improvement. Since we need to use async, I think we don't need this improvement
--
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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219592655
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -25,19 +25,58 @@ using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
+ Result producerResult =
+ client.createProducer("persistent://public/default/topic-non-batch", producerConfiguration, producer);
ASSERT_EQ(producerResult, ResultOk);
Consumer consumer;
- Result consumerResult =
- client.subscribe("persistent://public/default/testConsumeSuccess", "testConsumeSuccess", consumer);
+ Result consumerResult = client.subscribe("persistent://public/default/topic-non-batch", "sub", consumer);
ASSERT_EQ(consumerResult, ResultOk);
- const auto msg = MessageBuilder().setContent("testConsumeSuccess").build();
- Result sendResult = producer.send(msg);
- ASSERT_EQ(sendResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
+ ASSERT_EQ(sendResult, ResultOk);
+ }
+
+ Message receivedMsg;
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ printf("receive index: %d\n", i);
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_EQ(receivedMsg.getIndex(), i);
+ Result ackResult = consumer.acknowledge(receivedMsg);
+ ASSERT_EQ(ackResult, ResultOk);
+ }
+ client.close();
+}
+
+TEST(BrokerMetadataTest, testConsumeBatchSuccess) {
+ Client client{"pulsar://localhost:6650"};
+ Producer producer;
+ Result producerResult = client.createProducer("persistent://public/default/topic-batch", producer);
+ ASSERT_EQ(producerResult, ResultOk);
+ Consumer consumer;
+ Result consumerResult = client.subscribe("persistent://public/default/topic-batch", "sub", consumer);
+ ASSERT_EQ(consumerResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
Review Comment:
@BewareMyPower Fixed
--
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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1219830159
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -20,24 +20,74 @@
#include <gtest/gtest.h>
#include <pulsar/Client.h>
+#include "lib/Latch.h"
+
using namespace pulsar;
TEST(BrokerMetadataTest, testConsumeSuccess) {
Client client{"pulsar://localhost:6650"};
Producer producer;
- Result producerResult = client.createProducer("persistent://public/default/testConsumeSuccess", producer);
+ ProducerConfiguration producerConfiguration;
+ producerConfiguration.setBatchingEnabled(false);
+ Result producerResult =
+ client.createProducer("persistent://public/default/topic-non-batch", producerConfiguration, producer);
ASSERT_EQ(producerResult, ResultOk);
Consumer consumer;
- Result consumerResult =
- client.subscribe("persistent://public/default/testConsumeSuccess", "testConsumeSuccess", consumer);
+ Result consumerResult = client.subscribe("persistent://public/default/topic-non-batch", "sub", consumer);
ASSERT_EQ(consumerResult, ResultOk);
- const auto msg = MessageBuilder().setContent("testConsumeSuccess").build();
- Result sendResult = producer.send(msg);
- ASSERT_EQ(sendResult, ResultOk);
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ Result sendResult = producer.send(msg);
+ ASSERT_EQ(sendResult, ResultOk);
+ }
+
+ Message receivedMsg;
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ printf("receive index: %d\n", i);
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_EQ(receivedMsg.getIndex(), i);
+ Result ackResult = consumer.acknowledge(receivedMsg);
+ ASSERT_EQ(ackResult, ResultOk);
+ }
+ client.close();
+}
+
+TEST(BrokerMetadataTest, testConsumeBatchSuccess) {
+ Client client{"pulsar://localhost:6650"};
+ Producer producer;
+ Result producerResult = client.createProducer("persistent://public/default/topic-batch", producer);
+ ASSERT_EQ(producerResult, ResultOk);
+ Consumer consumer;
+ Result consumerResult = client.subscribe("persistent://public/default/topic-batch", "sub", consumer);
+ ASSERT_EQ(consumerResult, ResultOk);
+
+ Latch latch(10);
+ auto sendCallback = [&latch](Result result, const MessageId& id) {
+ ASSERT_EQ(result, ResultOk);
+ latch.countdown();
+ };
+
+ for (int i = 0; i < 10; i++) {
+ std::string content = "testConsumeSuccess" + std::to_string(i);
+ const auto msg = MessageBuilder().setContent(content).build();
+ producer.sendAsync(msg, sendCallback);
+ }
+
+ latch.wait();
+
Message receivedMsg;
- Result receiveResult = consumer.receive(receivedMsg);
- ASSERT_EQ(receiveResult, ResultOk);
- ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess");
+ for (int i = 0; i < 10; i++) {
+ Result receiveResult =
+ consumer.receive(receivedMsg, 1000); // Assumed that we wait 1000 ms for each message
+ ASSERT_EQ(receiveResult, ResultOk);
+ ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess" + std::to_string(i));
+ ASSERT_GE(receivedMsg.getIndex(), 0);
+ ASSERT_LT(receivedMsg.getIndex(), 10);
Review Comment:
@BewareMyPower Sorry, I missed push a commit. I will fix it after going to home.
--
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 #277: [feat][Message] Add getIndex method on Message
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277
--
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] Shoothzj commented on a diff in pull request #277: [feat][Message] Add getIndex method on Message
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1213380924
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -38,6 +38,7 @@ TEST(BrokerMetadataTest, testConsumeSuccess) {
Result receiveResult = consumer.receive(receivedMsg);
ASSERT_EQ(receiveResult, ResultOk);
ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess");
+ ASSERT_NE(receivedMsg.getIndex(), -1);
Review Comment:
@BewareMyPower Fixed
--
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 #277: [feat][Message] Add getIndex method on Message
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #277:
URL: https://github.com/apache/pulsar-client-cpp/pull/277#discussion_r1213110165
##########
include/pulsar/Message.h:
##########
@@ -124,6 +125,12 @@ class PULSAR_PUBLIC Message {
*/
void setMessageId(const MessageId& messageId) const;
+ /**
+ * Get the index of this message, if it doesn't exist, return -1
+ * @return
+ */
+ const int64_t getIndex() const;
Review Comment:
It's meaningless to return a const value.
```c++
const int f() { return 1; }
```
```c++
auto x1 = f(); // x1 is a non-const int
const auto x1 = f(); // x2 is a const int
```
You can see, even if you return a const int, if the returned variable is const is determined by the qualifier to the variable.
Things go different for reference, which is determined by the C++ type deduction rule.
```c++
const int& f() { static int x = 0; return x; }
```
```c++
auto& x1 = f(); // x1 is const int&
auto&& x2 = f(); // x2 is const int&&
```
##########
tests/brokermetadata/BrokerMetadataTest.cc:
##########
@@ -38,6 +38,7 @@ TEST(BrokerMetadataTest, testConsumeSuccess) {
Result receiveResult = consumer.receive(receivedMsg);
ASSERT_EQ(receiveResult, ResultOk);
ASSERT_EQ(receivedMsg.getDataAsString(), "testConsumeSuccess");
+ ASSERT_NE(receivedMsg.getIndex(), -1);
Review Comment:
Could you test sending N messages and verify the `getIndex()` calls return 0 to N-1. Currently it only tests `getIndex()` doesn't return an invalid value.
And you can also add an assertion to an existing test that `getIndex()` returns -1 when the interceptor is not configured.
##########
lib/Message.cc:
##########
@@ -136,6 +140,15 @@ void Message::setMessageId(const MessageId& messageID) const {
return;
}
+const int64_t Message::getIndex() const {
+ if (!impl_) {
+ return -1;
+ } else {
+ // casting uint64_t to int64_t, server definition ensures that's safe
+ return static_cast<int64_t>(impl_->brokerEntryMetadata.index());
Review Comment:
I think we should check `has_index()` as well because `index` is an optional field.
```c++
if (!impl_ || !impl_->has_index()) {
return -1;
```
##########
lib/MessageImpl.cc:
##########
@@ -20,7 +20,8 @@
namespace pulsar {
-MessageImpl::MessageImpl() : metadata(), payload(), messageId(), cnx_(0), topicName_(), redeliveryCount_() {}
+MessageImpl::MessageImpl()
+ : metadata(), brokerEntryMetadata(), payload(), messageId(), cnx_(0), topicName_(), redeliveryCount_() {}
Review Comment:
Actually we don't need this modification. All fields are initialized with default constructors.
--
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