Copilot commented on code in PR #583:
URL: https://github.com/apache/pulsar-client-cpp/pull/583#discussion_r3331940372
##########
lib/c/c_Message.cc:
##########
@@ -151,4 +151,13 @@ const char
*pulsar_message_get_producer_name(pulsar_message_t *message) {
return message->message.getProducerName().c_str();
}
+const char *pulsar_message_get_replicated_from(pulsar_message_t *message) {
+ const auto replicatedFrom = message->message.getReplicatedFrom();
+ if (!replicatedFrom) {
+ return NULL;
+ }
+
+ return message->message.getReplicatedFrom().value()->c_str();
+}
Review Comment:
This function calls `getReplicatedFrom()` twice and returns `NULL` from C++
code. Reuse the already-fetched optional and return `nullptr` to avoid
redundant work and keep null-pointer style consistent in this translation unit.
##########
include/pulsar/Message.h:
##########
@@ -220,6 +220,14 @@ class PULSAR_PUBLIC Message {
*/
const std::string& getProducerName() const noexcept;
+ /**
+ * Get the source cluster from which the message was replicated.
+ *
+ * @return the optional pointer to the source cluster name if the message
was replicated, the pointer is
+ * valid as the Message instance is alive
+ */
+ std::optional<const std::string*> getReplicatedFrom() const;
Review Comment:
This new accessor’s signature doesn’t match the API described in issue #569
(and referenced by the PR description). The issue requests `bool isReplicated()
const` and `const std::string& getReplicatedFrom() const` (empty when not
replicated), but this change introduces `std::optional<const std::string*>`,
which is a different contract and forces callers to deal with pointer lifetime
semantics.
##########
lib/c/c_Message.cc:
##########
@@ -151,4 +151,13 @@ const char
*pulsar_message_get_producer_name(pulsar_message_t *message) {
return message->message.getProducerName().c_str();
}
+const char *pulsar_message_get_replicated_from(pulsar_message_t *message) {
+ const auto replicatedFrom = message->message.getReplicatedFrom();
+ if (!replicatedFrom) {
+ return NULL;
+ }
+
+ return message->message.getReplicatedFrom().value()->c_str();
+}
Review Comment:
This function calls `getReplicatedFrom()` twice and returns `NULL` from C++
code. Reuse the already-fetched optional and return `nullptr` to avoid
redundant work and keep null-pointer style consistent in this translation unit.
##########
lib/Message.cc:
##########
@@ -239,6 +239,13 @@ const std::string& Message::getProducerName() const
noexcept {
return impl_->metadata.producer_name();
}
+std::optional<const std::string*> Message::getReplicatedFrom() const {
+ if (!impl_ || !impl_->metadata.has_replicated_from()) {
+ return std::nullopt;
+ }
+ return &impl_->metadata.replicated_from();
+}
Review Comment:
`replicated_from` being present but empty (possible with protobuf optional
string) currently returns a non-null value, which would make callers treat the
message as replicated even though the cluster name is empty. This also
conflicts with the C API contract that returns NULL when not replicated. Treat
an empty `replicated_from` as “not replicated”.
##########
include/pulsar/Message.h:
##########
@@ -220,6 +220,14 @@ class PULSAR_PUBLIC Message {
*/
const std::string& getProducerName() const noexcept;
+ /**
+ * Get the source cluster from which the message was replicated.
+ *
+ * @return the optional pointer to the source cluster name if the message
was replicated, the pointer is
+ * valid as the Message instance is alive
+ */
+ std::optional<const std::string*> getReplicatedFrom() const;
Review Comment:
This new accessor’s signature doesn’t match the API described in issue #569
(and referenced by the PR description). The issue requests `bool isReplicated()
const` and `const std::string& getReplicatedFrom() const` (empty when not
replicated), but this change introduces `std::optional<const std::string*>`,
which is a different contract and forces callers to deal with pointer lifetime
semantics.
##########
lib/Message.cc:
##########
@@ -239,6 +239,13 @@ const std::string& Message::getProducerName() const
noexcept {
return impl_->metadata.producer_name();
}
+std::optional<const std::string*> Message::getReplicatedFrom() const {
+ if (!impl_ || !impl_->metadata.has_replicated_from()) {
+ return std::nullopt;
+ }
+ return &impl_->metadata.replicated_from();
+}
Review Comment:
`replicated_from` being present but empty (possible with protobuf optional
string) currently returns a non-null value, which would make callers treat the
message as replicated even though the cluster name is empty. This also
conflicts with the C API contract that returns NULL when not replicated. Treat
an empty `replicated_from` as “not replicated”.
--
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]
