[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #148: [feat] PIP 107: Introduce chunk message ID

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

BewareMyPower commented on code in PR #148:
URL: https://github.com/apache/pulsar-client-cpp/pull/148#discussion_r1052081373


##########
include/pulsar/Message.h:
##########
@@ -188,8 +188,7 @@ class PULSAR_PUBLIC Message {
     MessageImplPtr impl_;
 
     Message(MessageImplPtr& impl);
-    Message(const proto::CommandMessage& msg, proto::MessageMetadata& data, SharedBuffer& payload,
-            int32_t partition);
+    Message(const MessageId& messageId, proto::MessageMetadata& metadata, SharedBuffer& payload);

Review Comment:
   Just have a thought, not a requested change and this PR needs no change about this comment. I think the constructor of `Message` should have only one private constructor.
   
   ```c++
   Message(const MessageImplPtr& impl);
   ```
   
   The other constructors should be moved to `MessageImpl`. After that, there is no need to expose the classes (even if as the forward declarations) in the header.



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