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

Refactor dispatcher's getting next consumer with consumer priority #367

Merged
merged 2 commits into from
Apr 20, 2017

Conversation

rdhabalia
Copy link
Contributor

Motivation

Refactoring #364 to have simplicity in finding next-available consumer at dispatcher by considering consumer-priority level.

Result

No functional change.

@rdhabalia rdhabalia added type/bug The PR fixed a bug or issue reported a bug type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Apr 19, 2017
@rdhabalia rdhabalia added this to the 1.18 milestone Apr 19, 2017
@rdhabalia rdhabalia self-assigned this Apr 19, 2017
do {
int priorityLevel = consumerList.get(currentConsumerRoundRobinIndex).getPriorityLevel()
- consumerList.get(resultingAvailableConsumerIndex).getPriorityLevel();
int currentRoundRobinConsumerPriority = consumerList.get(currentConsumerRoundRobinIndex).getPriorityLevel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep getNextConsumer() as private/protected method? This and other utility method assumes we are in synchronization block. Else array location could be invalid when it changes behind the scene

Copy link
Contributor Author

@rdhabalia rdhabalia Apr 19, 2017

Choose a reason for hiding this comment

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

yes, updated it with private.

*/
private int getNextConsumerFromSameOrLowerLevel(int currentRoundRobinIndex) {

int targetPriority = consumerList.get(currentConsumerRoundRobinIndex).getPriorityLevel();
Copy link
Contributor

Choose a reason for hiding this comment

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

currentRoundRobinIndex instead of currentConsumerRoundRobinIndex

@rdhabalia rdhabalia force-pushed the cons_priority_simple branch 2 times, most recently from ba78708 to 7f2d472 Compare April 19, 2017 23:32
*/
private int getNextConsumerFromSameOrLowerLevel(int currentRoundRobinIndex) {

int targetPriority = consumerList.get(currentRoundRobinIndex).getPriorityLevel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we break this into a sub method to give a consumer for a specific priority? e.g: getConsumerForPriority(priority, currentRRIndex)?
It should wrap around for that priority(if in the middle of the priority list) and return the index for available consumer or -1.
We can add a for loop for each remaining priority from currentPriority till maxPriority known to us).

Something like below:
foreach(priority = consumerList.get(currentRoundRobinIndex).getPriorityLevel(); priority<consumerList.get(consumerList.size()-1).getPriorityLevel(); priority++) {
consumerIndex = getConsumerForPriority(priority, currentRRIndex);
if( consumerIndex != -1) {
return consumerIndex;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed: getNextConsumerFromSameOrLowerLevel checks consumer from current-level and saves the index of the next level in case we don't find consumer in the current-level and then does linear search in lower-level which is one loop so, as we discuss we will keep it in the same method.

Copy link
Contributor

@saandrews saandrews left a comment

Choose a reason for hiding this comment

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

The current logic should also work.

@rdhabalia rdhabalia force-pushed the cons_priority_simple branch from 7f2d472 to 6dee55b Compare April 20, 2017 01:31
@rdhabalia
Copy link
Contributor Author

@merlimat can you review it when you get a chance as thinking to pick this one over #364

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM

@rdhabalia rdhabalia merged commit 5862644 into apache:master Apr 20, 2017
@rdhabalia rdhabalia deleted the cons_priority_simple branch June 21, 2017 18:53
hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020
…s enabled (apache#372)

Fixes apache#367 


### Motivation

Send delay message individually even batching is enabled.


### Modifications

1. flush batching messages immediately when a new delay message is received
2. reset deliverAtTime metadata of `BatchBuilder`
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 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants