-
Notifications
You must be signed in to change notification settings - Fork 373
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 remote configuration worker #2691
Conversation
Pending:
|
e6e6425
to
6d0e56b
Compare
08eff6e
to
cc025a9
Compare
Rebased using |
6d0e56b
to
175e9bd
Compare
cc025a9
to
59f4de9
Compare
Rebased using |
db9c31e
to
5220533
Compare
59f4de9
to
39ec14e
Compare
39ec14e
to
42b74f7
Compare
Rebased using |
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 a few things could be improved
a6d21d2
to
748ee58
Compare
expect(worker).to receive(:acquire_lock).at_least(:once) | ||
expect(worker).to receive(:release_lock).at_least(:once) |
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.
at_least(:once)
because calling worker.stop
, which happen on the after block, will also call acquire_lock && release_lock
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.
Consider perhaps making the mutex a dependency that can be injected (via the constructor) to avoid having to set expectations on the class being tested.
Something along the lines of
class Worker
def initialize(interval:, mutex: Mutex.new, &block)
@mutex = mutex
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 had it that way before.
I don't mind having to set expectations for the class being tested. Especially if the API does not break, if in the future we use a different class than mutex, we could change the underlaying lock mechanism without having to change the class initialise arguments or test.
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.
Left a few notes, nothing blocking! :)
def poll(interval) | ||
loop do | ||
break unless @mutex.synchronize { @starting || @started } | ||
|
||
call | ||
|
||
sleep(interval) | ||
end | ||
end |
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.
Minor: Since this runs on a background thread, you may want to log if it exits with an exception
|
||
@starting = true | ||
|
||
@thr = Thread.new { poll(@interval) } |
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.
Minor: Consider naming this thread (on Ruby 2.3+) -- this will show up in the profiler clearly, as well as in other debugging tools
expect(worker).to receive(:acquire_lock).at_least(:once) | ||
expect(worker).to receive(:release_lock).at_least(:once) |
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.
Consider perhaps making the mutex a dependency that can be injected (via the constructor) to avoid having to set expectations on the class being tested.
Something along the lines of
class Worker
def initialize(interval:, mutex: Mutex.new, &block)
@mutex = mutex
it 'runs block' do | ||
worker.start | ||
# Wait for the work task to execute once | ||
queue.pop | ||
expect(result).to eq([1]) | ||
end |
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.
Minor: Having both queue
and result
seems a bit redundant -- consider maybe using expect(queue.pop).to be 1
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.
Thanks for the suggestion. Will make sure to change it in a follow up PR
@ivoanjo Thanks for all the suggestions. I'm going to go ahead and merge it, but I will get back to those in a follow-up PR 😄 |
Yup, sounds good :) |
What does this PR do?
Implement a polling worker for remote configuration.
Motivation
Remote configuration
Additional Notes
This is a simple generic polling worker:
new
and it'll call it everyinterval
There is no fork protection: it is supposed to start on demand when the first request comes in.
How to test the change?
Example to use it in conjunction with the remote configuration client is below.
Note that the agent may not reply immediately with a full configuration, but it happens eventually.