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