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

Queue impl parity #79

Merged
merged 5 commits into from
Dec 28, 2021

Conversation

marcoferrer
Copy link
Contributor

@marcoferrer marcoferrer commented Dec 28, 2021

This PR creates a new limiter type named QueueBlockingLimiter. This limiter is meant to replace LifoBlockingLimiter as well as FifoBlockingLimiter. This new queue takes all the improvements made to the lifo queue implementation as well as refactors the queue implementation to use a List internally, just like the fifo queue impl. The queue direction is controlled via configuration enum passed in at instantiation time. The existing queue tests have been ported over. If this proposal is acceptable then one thing I would like a little guidance on is what course of action to take for the existing queues and pool implementations? Would it make sense to fully deprecate the old methods and wait a few release cycles or remove them completely and release a breaking change.

EDIT: I used a combination of type aliasing and embedding to deprecate the older queue limiter types

@@ -11,7 +11,7 @@ import (

type lifoElement struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes were included because I branched off #78

If that PR gets merged I can rebase and this diff should be resolved

Comment on lines 94 to 102
var element *list.Element
switch q.ordering {
case OrderingFIFO:
element = q.list.Back()
case OrderingLIFO:
element = q.list.Front()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is essentially the biggest difference between this impl and what exists in the lifo impl. It is also the only code path where we have any logic dependent on the queue order

@@ -0,0 +1,474 @@
package limiter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these tests were sourced from lifo_blocking_test.go and fifo_blocking_test.go

@marcoferrer marcoferrer marked this pull request as ready for review December 28, 2021 18:39
@marcoferrer marcoferrer force-pushed the queue-impl-parity branch 3 times, most recently from ed5b5fe to 92dcf34 Compare December 28, 2021 19:10
@platinummonkey
Copy link
Owner

looks good, mind addressing these go fmt errors? 🙏 https://github.com/platinummonkey/go-concurrency-limits/runs/4652860617?check_suite_focus=true

@marcoferrer
Copy link
Contributor Author

looks good, mind addressing these go fmt errors? 🙏 https://github.com/platinummonkey/go-concurrency-limits/runs/4652860617?check_suite_focus=true

No prob. Missed those. Ill take care of it now

Copy link
Owner

@platinummonkey platinummonkey left a comment

Choose a reason for hiding this comment

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

👏 nice work, thanks for all the contributions!

@platinummonkey platinummonkey merged commit 92df8ad into platinummonkey:master Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants