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

Add pending-blocked-message-permits to dispatcher while redelivery of pending messages #63

Merged
merged 1 commit into from
Oct 17, 2016

Conversation

rdhabalia
Copy link
Contributor

Motivation

When Consumer is blocked due to not acking max-unack_threshold messages: Consumer keeps adding flow-permits to messagePermits and doesn't sends it to Dispatcher until consumer gets unblocked again.
In Mean time, if redelivery requests comes then Dispatcher doesn't send additional number of messages for permits which came after consumer was blocked.

  • So, we need two separate counts

(1) messagePermit before consumer blocked
(2) messagePermit after consumer blocked.

So, when consumer receives redelivery request: it can redeliver number of blocked-permit messages in message-replay.

Modifications

Introduce two message permit counts to support redelivery of messages when consumer is blocked and client-consumer actually has more permits to receive more messages.

Result

It delivers messages on redelivery-message request by also considering permits which comes after consumer got blocked due to reaching unack-message-threshold.

@yahoocla
Copy link

CLA is valid!

2 similar comments
@yahoocla
Copy link

CLA is valid!

@yahoocla
Copy link

CLA is valid!

@rdhabalia rdhabalia added the type/bug The PR fixed a bug or issue reported a bug label Oct 14, 2016
@rdhabalia rdhabalia added this to the 1.15 milestone Oct 14, 2016
@rdhabalia rdhabalia self-assigned this Oct 14, 2016
@rdhabalia rdhabalia changed the title Add pending-bloclked-message-permits to dispatcher while redelivery of pending messages Add pending-blocked-message-permits to dispatcher while redelivery of pending messages Oct 14, 2016
Copy link
Contributor

@sboobna sboobna left a comment

Choose a reason for hiding this comment

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

Just some minor stuff, else the change looks good

// dispatch pending permits to flow more messages: it will add more permits to dispatcher and consumer
subscription.consumerFlow(consumer, additionalNumberOfPermits);
// add newly flow permits to actual consumer.messagePermits
consumer.messagePermits.getAndAdd(additionalNumberOfPermits);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the messagePermits before sending the flow just to be consistent

subscription.consumerFlow(this, additionalNumberOfMessages);
}else {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed both the comments.. thanks.

Copy link
Contributor

@sboobna sboobna left a comment

Choose a reason for hiding this comment

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

👍
@merlimat could you take a look at this change?

@merlimat
Copy link
Contributor

Looks good, but wanted to check it with fresh eyes tomorrow

@rdhabalia rdhabalia force-pushed the unack_block branch 2 times, most recently from 1468f6d to b07112b Compare October 17, 2016 17:52
@rdhabalia
Copy link
Contributor Author

@merlimat : addressed renaming changes.

@merlimat merlimat merged commit 5b7b910 into apache:master Oct 17, 2016
@rdhabalia rdhabalia deleted the unack_block branch November 11, 2016 23:02
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
Fixes apache#63

*Movitation*

Migrate the jenkins to the Github Action.
nicoloboschi referenced this pull request in nicoloboschi/pulsar Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants