AnonHxy commented on issue #16680:
URL: https://github.com/apache/pulsar/issues/16680#issuecomment-1196210743
Thank you for your patience @asafm . Yes, what you described above is
correctly.
> All I wrote above is not clearly stated in the PIP. IMO the PIP needs to
be modified to reflect that explanation.
I will give a more detailed explanation in the PIP
> One option may be to rename `batchedFilterProperties` to
`batchGroupByProperties` so they will know the batching behavior is changing
I agree with you. `batchGroupByProperties` is better, which indicates
batching behavior.
> Don't you need to introduce new knobs to control the memory?
No. There is no need to take care of controling the memory in this PIP.
Because the groupBy behavior will be done in the `BatchMessageContainerImpl`,
which will control the memory.
For details:
We can add a method named `hasSameProperties` in
`BatchMessageContainerImpl`, which will looks like the exsited method
`BatchMessageContainerImpl#hasSameSchema`:
```
public boolean hasSameProperties(MessageImpl<?> msg) {
if (numMessagesInBatch == 0) {
return true;
}
if (!messageMetadata.getPropertiesList().isEmpty()) {
return getProperties(msg).isEmpty();
}
return
getProperties(msg).equals(messageMetadata.getPropertiesList());
}
```
And `hasSameProperties` will be invoked when producer try to add messages in
the `BatchMessageContainerImpl`:
```
private boolean canAddToCurrentBatch(MessageImpl<?> msg) {
return batchMessageContainer.haveEnoughSpace(msg) // memory
control is here
&& (!isMultiSchemaEnabled(false) ||
batchMessageContainer.hasSameSchema(msg))
&& batchMessageContainer.hasSameProperties(msg) //
invoke it here
&& batchMessageContainer.hasSameTxn(msg);
}
```
As we see above the `batchMessageContainer` has already checked the space.
--
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]
