Bootstrap first gem version #1
Conversation
Used `bundler gem warm-blanket` to generate template and slightly tweaked it to our standards.
This class takes a list of endpoints and makes a request to one of them (in order) every time it is called.
This class waits for a given port to be open on the target machine.
lib/warm-blanket.rb
Outdated
| end | ||
| end | ||
|
|
||
| WarmBlanket.instance_eval do |
There was a problem hiding this comment.
Couldn't this be included in the above module definition?
module WarmBlanket
extend Dry::Configurable
setting ...
def self.trigger_warmup(...)
end
endThere was a problem hiding this comment.
Yes indeed. I got confused by the documentation and thought that maybe it did not work as expected on modules, but re-testing the assumption it turns out that is not the case.
Fixed in 65fe3d2
Due to a misunderstanding of dry-configuration documentation I believed that we could not use their module and configuration on a `module` rather than a `class`; but after a review suggestion it turns out that it works, so we can simplify the configuration.
The two fixed methods are instance methods, not class methods, and thus they should be prefixed with `#` and not with `.`.
Otherwise, we would fail when picking up the port from an environment variable.
|
|
||
| tries = 0 | ||
|
|
||
| while true |
There was a problem hiding this comment.
Would a default timeout make sense here? Asking because due to the current state of affairs, we really don't want this to still be running when the new dynos start receiving production traffic.
There was a problem hiding this comment.
I tried to approximate it with a attempts counter, but I can switch to a time-based scheme, that indeed would give us better guarantees of how much time is spent here.
lib/warm_blanket/orchestrator.rb
Outdated
| end | ||
| end | ||
|
|
||
| def wait_for_port_to_open |
There was a problem hiding this comment.
What do you think of renaming this to port_is_open?? Or port_is_finally_open? :P
Asking because this is being used in the if block above, and it's a bit weird to read if wait_for_port_to_open. Aka, nitpicking 💅
There was a problem hiding this comment.
Hmm. I see your point, but I wanted to highlight that this is a non-trivial operation that may take time.
What do you think if I instead changed #orchestrate to be
def orchestrate
success = wait_for_port_to_open
spawn_warmup_threads if success
endThus eliminating the confusing if?
lib/warm_blanket/requester.rb
Outdated
|
|
||
| logger.debug "Requesting #{endpoint.fetch(:get)}" | ||
|
|
||
| response = connection.get do |request| |
There was a problem hiding this comment.
Flexibility over the verb is desirable (e.g.: Permissions API requires a POST).
There was a problem hiding this comment.
I was trying to get away with just get on the first version, will do.
There was a problem hiding this comment.
Although we'll have to be careful with any side effects caused by such a post.
There was a problem hiding this comment.
Feature added in 132830b
This way we remove a strange blocking operation from the if, and make it clear that this may take some time.
|
LGTM! Merge away! 👍 |
|
Thanks! Merging ahead, and I'll create an issue to track the time-based waiting for port work separately. |
See individual commits AND the beautiful
READMEI wrote for details :)