Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken main branch by providing correct getBitSet implementation #175

Closed

Conversation

BewareMyPower
Copy link
Contributor

Motivation

Currently the main branch is broken by the concurrent merge of #153 and #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.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

### 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.
@BewareMyPower BewareMyPower self-assigned this Jan 17, 2023
@BewareMyPower BewareMyPower added the bug Something isn't working label Jan 17, 2023
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Jan 17, 2023
@shibd
Copy link
Member

shibd commented Jan 17, 2023

I have a question:

It's because when a batched message id is constructed from deserialization, there is no getBitSet implementation of the internal acker.

Is the code related to this passage here?

if (impl_->batchIndex_ >= 0 && impl_->batchSize_ > 0) {
return MessageId{std::make_shared<BatchedMessageIdImpl>(*impl_, BatchMessageAckerImpl::create(0))};
} else {

Is setting bitSize equal to 0 for lazy initialization? To save memory?

If bitSite is initialized here, subsequent operations such as ack can reuse the logic here. Wouldn't it be better?

bool ackIndividual(int32_t batchIndex) const { return acker_->ackIndividual(batchIndex); }
bool ackCumulative(int32_t batchIndex) const { return acker_->ackCumulative(batchIndex); }

@@ -37,14 +37,16 @@ class BatchMessageAcker {
// by deserializing from raw bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modification of this class to have solved the compilation error. Can we first open a new PR to merge it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will modify this PR after #177 is merged.

@BewareMyPower BewareMyPower marked this pull request as draft January 18, 2023 09:10
@shibd shibd modified the milestones: 3.2.0, 3.3.0 Jun 19, 2023
@BewareMyPower BewareMyPower modified the milestones: 3.3.0, 3.4.0 Jul 25, 2023
@BewareMyPower BewareMyPower modified the milestones: 3.4.0, 3.5.0 Nov 1, 2023
@BewareMyPower BewareMyPower removed this from the 3.5.0 milestone Mar 1, 2024
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-broken-master branch June 11, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants