[GitHub] [flink-connector-pulsar] BewareMyPower commented on a diff in pull request #54: [FLINK-32612] Upgrade Pulsar version to 3.0.0

["BewareMyPower (via GitHub)" <gi...@apache.org>]

BewareMyPower commented on code in PR #54:
URL: https://github.com/apache/flink-connector-pulsar/pull/54#discussion_r1266104763


##########
flink-connector-pulsar/src/main/java/org/apache/flink/connector/pulsar/source/enumerator/cursor/stop/MessageIdStopCursor.java:
##########
@@ -43,8 +45,12 @@ public MessageIdStopCursor(MessageId messageId, boolean inclusive) {
         checkArgument(!earliest.equals(messageId), "MessageId.earliest is not supported.");
         checkArgument(!latest.equals(messageId), "Use LatestMessageStopCursor instead.");
 
-        this.messageId = convertToMessageIdImpl(messageId);
         this.inclusive = inclusive;
+        try {
+            this.messageId = MessageId.fromByteArray(messageId.toByteArray());
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }

Review Comment:
   If you only need a `MessageIdImpl`, you can just use `MessageIdImpl` here to make it clear that the MessageId you want represents exactly a non-batched message.
   
   But `MessageIdAdv` is a more stable interface that was introduced by a formal PIP, while `MessageIdImpl` is an implementation class. Using a class instead of an interface in ecosystem might make the upstream side hard to do some refactoring.
   
   If you want to perform a validation that a message ID of a batch message is not allowed here, just check `msgIdAdv.getBatchIndex()`.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org