[fix] Fix MessageId serialization when it's a batched message #153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
### Motivation The serialization and deserialization of `MessageId` became wrong after apache#132. 1. The batch size is not serialized. 2. `BatchedMessageIdImpl` could never be deserialized. The wrong behaviors could lead to a result that all MessageId objects created from deserialization does not have a batch size, which might make `ReaderTest.testReaderOnSpecificMessageWithBatches` fail when the cmake build type is `Debug`. What's worse is that a MessageId created from deserialization is always treated as a `MessageIdImpl`, on which the acknowledgment will have wrong behavior. ### Modifications Serialize the batch size if it's valid. In deserialization, create a `BatchedMessageIdImpl` when the batch index and the batch size are valid as a batched message. There is a problem that if a `MessageId` is created from deserialization, it cannot share a `BatchMessageAcker` with other `MessageId` objects. In this case, create a fake `BatchMessageAcker` that returns false for both `ackIndividual` and `ackCumulative` methods. It will make acknowledgment always fail but will fall back to batch index ACK if batch index ACK is enabled. Add the `-DCMAKE_BUILD_TYPE=Debug` for tests to enable assertions.
|
@BewareMyPower It seems that compilation does not pass in the macOS environment. |
|
@shibd I see. I will fix it soon. |
shibd
approved these changes
Dec 23, 2022
|
@merlimat @RobertIndie @Demogorgon314 Could you take a second look? |
Demogorgon314
approved these changes
Dec 28, 2022
RobertIndie
approved these changes
Dec 28, 2022
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Jan 17, 2023
### Motivation Currently the main branch is broken by the concurrent merge of apache#153 and apache#151. It's because when a batched message id is constructed from deserialization, there is no `getBitSet` implementation of the internal acker. ### Modifications Add a `bool` parameter to `MessageIdImpl::getBitSet` to indicate whether the message ID is batched. The logic is similar with https://github.com/apache/pulsar/blob/299bd70fdfa023768e94a8ee4347d39337b6cbd4/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L325-L327 and https://github.com/apache/pulsar/blob/299bd70fdfa023768e94a8ee4347d39337b6cbd4/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L345-L347 Add a `testMessageIdFromBuild` to test the acknowledgment for a message ID without an acker could succeed for a consumer that enables batch index ACK. ### TODO In future, apache/pulsar#19031 might be migrated into the C++ client to fix the consumer that disables batch index ACK.
4 tasks
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Jan 18, 2023
### Motivation Currently the main branch is broken by the concurrent merge of apache#153 and apache#151. ### Modifications Add a dummy `getBitSet` implementation to `BatchMessageAcker` and the correct implementation for `BatchMessageAckerImpl`.
4 tasks
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Oct 27, 2024
Fix the regression introduced in apache#153 ---- 9.006, 8.860, 9.113
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The serialization and deserialization of
MessageIdbecame wrong after #132.BatchedMessageIdImplcould never be deserialized.The wrong behaviors could lead to a result that all MessageId objects created from deserialization does not have a batch size, which might make
ReaderTest.testReaderOnSpecificMessageWithBatchesfail when the cmake build type isDebug. What's worse is that a MessageId created from deserialization is always treated as aMessageIdImpl, on which the acknowledgment will have wrong behavior.Modifications
Serialize the batch size if it's valid. In deserialization, create a
BatchedMessageIdImplwhen the batch index and the batch size are valid as a batched message.There is a problem that if a
MessageIdis created from deserialization, it cannot share aBatchMessageAckerwith otherMessageIdobjects. In this case, create a fakeBatchMessageAckerthat returns false for bothackIndividualandackCumulativemethods. It will make acknowledgment always fail but will fall back to batch index ACK if batch index ACK is enabled.Add the
-DCMAKE_BUILD_TYPE=Debugfor tests to enable assertions.