Skip to content

Conversation

@AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented May 21, 2022

Motivation

  • It should fast return if ack type is Cumulative with illegal CommandAck

Modifications

  • Return if ack.getMessageIdsCount() != 1 or Subscription.isIndividualAckMode(subType)

Verifying this change

  • Make sure that the change passes the CI checks.
  • Add UT for this case org.apache.pulsar.broker.service.MessageCumulativeAckTest

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 21, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 21, 2022

/pulsarbot run-failure-checks

@AnonHxy AnonHxy changed the title [broker]Fast return if ack cumulative illegal [fix][broker]Fast return if ack cumulative illegal May 22, 2022
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

The change looks good. Could you please help add a test?

For the changes of line:382, it should be a bug fix and the behavior changed?
Before this change, if cumulative ack happened on a Shared subscription, the ack will take effect, but after this change, the ack will not take effect if the request with only one ack.

if (positions.size() != 1) {
log.warn("[{}][{}] Invalid cumulative ack received with multiple message ids.", topicName, subName);
return;
}
Position position = positions.get(0);
if (log.isDebugEnabled()) {
log.debug("[{}][{}] Cumulative ack on {}", topicName, subName, position);
}
cursor.asyncMarkDelete(position, mergeCursorProperties(properties),
markDeleteCallback, previousMarkDeletePosition);

@codelipenghui codelipenghui added this to the 2.11.0 milestone May 22, 2022
@codelipenghui codelipenghui added release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug area/broker labels May 22, 2022
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 22, 2022

Add UT, PTAL @codelipenghui

@AnonHxy AnonHxy force-pushed the fix_ack_cumulative branch from a6891ed to e94f1fd Compare May 22, 2022 18:48
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 23, 2022

/pulsarbot run-failure-checks

2 similar comments
@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 23, 2022

/pulsarbot run-failure-checks

@AnonHxy
Copy link
Contributor Author

AnonHxy commented May 23, 2022

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 02dca31 into apache:master Jun 6, 2022
codelipenghui pushed a commit to codelipenghui/incubator-pulsar that referenced this pull request Jun 7, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
(cherry picked from commit 02dca31)
(cherry picked from commit 5bf69dc)
codelipenghui pushed a commit that referenced this pull request Jun 10, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants