BewareMyPower commented on code in PR #18405:
URL: https://github.com/apache/pulsar/pull/18405#discussion_r1018769369
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java:
##########
@@ -155,8 +155,14 @@ public static MessageId fromByteArrayWithTopic(byte[]
data, TopicName topicName)
MessageId messageId;
if (idData.hasBatchIndex()) {
- messageId = new BatchMessageIdImpl(idData.getLedgerId(),
idData.getEntryId(), idData.getPartition(),
- idData.getBatchIndex(), idData.getBatchSize(),
BatchMessageAcker.newAcker(idData.getBatchSize()));
+ if (idData.hasBatchSize()) {
Review Comment:
And when I revisited the existing code, there are many places that set the
`batch_index` field but doesn't set the `batch_size` field. The `batch_size`
field is easily to be ignored because it's not considered in `equals` and
`hashCode` methods. BTW, I'm not sure whether the default value of the
`batch_index` field might increase the cases that `batch_index` is set while
`batch_size` is not set. IMO, adding default values to the optional fields
increases the chance of such an error. We should be careful for new fields if
it's optional.
--
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]
