Skip to content

Conversation

AndreSteenbergen
Copy link
Contributor

Added an UnboundStablePriorityMailbox, sending messages according to priority. Messages with the same priority will be send using the same order as they appear. #2652 is the related issue.

AndreSteenbergen and others added 2 commits July 5, 2018 22:01
…priority. Messages with the same priority will be send using the same order as they appear. akkadotnet#2652 is the related issue.
@Aaronontheweb Aaronontheweb self-assigned this Mar 20, 2019
@Aaronontheweb Aaronontheweb self-requested a review March 20, 2019 20:45
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Overall this looks really good but we need to add some unit tests, just to be on the safe side.

@@ -682,7 +682,7 @@ public override IMessageQueue Create(IActorRef owner, ActorSystem system)
/// Priority mailbox base class; unbounded mailbox that allows for prioritization of its contents.
/// Extend this class and implement the <see cref="PriorityGenerator"/> method with your own prioritization.
/// The value returned by the <see cref="PriorityGenerator"/> method will be used to order the message in the mailbox.
/// Lower values will be delivered first. Messages ordered by the same number will remain in delivery order.
/// Lower values will be delivered first. Messages ordered by the same number will remain in delivered in undefined order.
Copy link
Member

Choose a reason for hiding this comment

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

Right

/// Priority mailbox - an unbounded mailbox that allows for prioritization of its contents.
/// Extend this class and implement the <see cref="PriorityGenerator"/> method with your own prioritization.
/// The value returned by the <see cref="PriorityGenerator"/> method will be used to order the message in the mailbox.
/// Lower values will be delivered first. Messages ordered by the same number will remain in delivery order.
Copy link
Member

Choose a reason for hiding this comment

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

Looks good

/// <inheritdoc cref="MailboxType"/>
public sealed override IMessageQueue Create(IActorRef owner, ActorSystem system)
{
return new UnboundedStablePriorityMessageQueue(PriorityGenerator, InitialCapacity);
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

{
/// <summary>
/// Priority queue implemented using a simple list with binary search for inserts.
/// This specific implementation is cheap in terms of memory but weak in terms of performance.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comment, this is helpful.

@AndreSteenbergen
Copy link
Contributor Author

I actually forgot I made this pull request. I will try to add unit tests asap. I do have some other priorities as well. If I am correct I am using this in production somewhere already ...

@Aaronontheweb
Copy link
Member

@AndreSteenbergen all good - I added some myself. Had an urgent need for this feature and figured I could get it into a 1.3.13 release before I pull all of the v1.4 stuff into the dev branch.

@Aaronontheweb
Copy link
Member

@AndreSteenbergen good news is that all of your code worked perfectly - threw a Property-based test at the queue structure itself to verify it and some tests on the mailbox too. All worked as expected.

@ismaelhamed
Copy link
Member

@AndreSteenbergen all good - I added some myself. Had an urgent need for this feature and figured I could get it into a 1.3.13 release before I pull all of the v1.4 stuff into the dev branch.

I ain't installing anything 1.3.13 in production...

@Aaronontheweb
Copy link
Member

I ain't installing anything 1.3.13 in production...

LOL :p - maybe I'll do what the airlines do and skip over having seat 13 and go straight to 14 instead :p

@Aaronontheweb Aaronontheweb merged commit a4876ff into akkadotnet:dev Mar 21, 2019
@AndreSteenbergen
Copy link
Contributor Author

You can surely publish 1.3.13, just don't publish it on Friday the 13th ;)

madmonkey pushed a commit to madmonkey/akka.net that referenced this pull request Jul 12, 2019
* Added an UnboundStablePriorityMailbox, sending messages according to priority. Messages with the same priority will be send using the same order as they appear. akkadotnet#2652 is the related issue.

* added PropertyTest for StableListPriorityQueue

* forgot to add spec

* verified that the UnboundedStablePriorityMailbox can be loaded and used

* validate4d that UnboundedStablePriorityMailbox supports stashing

* added API approval for core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants