-
Notifications
You must be signed in to change notification settings - Fork 27
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
Decouple queue instantiation and population #34
Conversation
@@ -7,8 +7,8 @@ def initialize(redis:, build_id:) | |||
@build_id = build_id | |||
end | |||
|
|||
def empty? | |||
size == 0 | |||
def exhausted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed empty?
to exhausted?
to more clearly reflect that the queue is empty because it was fully worked off, and not because it's not populated yet.
Not sure why, but the python test suite is failing:
@solackerman any idea? |
@casperisfine try bumping this version number? |
That fixed it thanks. |
04c356e
to
994358e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ruby/lib/ci/queue/redis/base.rb
Outdated
def master_elected? | ||
@master_elected ||= begin | ||
status = master_status | ||
status == 'ready' || status == 'finished' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider the master elected after the queue is populated (state == ready) or when a worker gets the lock to begin pushing the queue (state == setup)? Just want to make sure master_elected?
is what you intend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the master is indeed already elected when the status is set to setup
.
However at that point the queue isn't filled, so yeah maybe that method should be along the lines ofmaster_elected_and_queue_initialized?
but it's seem a bit verbose.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just queue_initialized?
.That implies that a master has been elected as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
@@ -1,12 +1,36 @@ | |||
module SharedQueueAssertions | |||
class TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have a test case class in the actual library, rather than just for test purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fake class. Since we're about to support multiple test frameworks, the queue implementations don't know what kind of objects they contain.
However we could expect an interface, and the different implementations would have to wrap whatever test case objects they have in some kind of delegate object to translate to and from string.
994358e
to
1ff6b45
Compare
Fix: #19
This allows to check if the queue was exhausted without having to boot the entire test environment, which should allow a late worker to exit faster.
This PR also change the queue interface. Queues now receive and return arbitrary objects instead of strings. This will make it easier to implement adapters for test frameworks others than Minitest.