[Refactor] Use local configuration instead of a global one #41
Conversation
drewhoskins-stripe
left a comment
drewhoskins-stripe
left a comment
There was a problem hiding this comment.
Bullish on the direction!
Looks good as far as I can tell, though could use some documentation.
| attr_reader :name, :domain, :task_list, :retry_policy, :timeouts, :headers | ||
|
|
||
| def initialize(object, options = {}) | ||
| def initialize(object, options, defaults = nil) |
There was a problem hiding this comment.
Can you add a comment as to what the shape of defaults is and what to pass in?
There was a problem hiding this comment.
It is an instance of Cadence::Configuration::Execution, but I see your point, this is immediately visible and can cause confusion. Agree that we need to address it
| MAX_FAILED_ATTEMPTS = 50 | ||
|
|
||
| def initialize(task, domain, workflow_lookup, client, middleware_chain) | ||
| def initialize(task, domain, workflow_lookup, connection, middleware_chain, config) |
There was a problem hiding this comment.
Would love to see some comments in at least some of these classes, like what's a connection, what's a config, or how do I get them to pass in?
There was a problem hiding this comment.
Yes, point taken, will think of a solution 👍
lib/cadence/workflow/poller.rb
Outdated
|
|
||
| def process(task) | ||
| client = Cadence::Client.generate | ||
| connection = Cadence::Connection.generate(config.for_connection) |
There was a problem hiding this comment.
could/should this be done inside of DecisionTaskProcessor.initialize?
There was a problem hiding this comment.
We can do this, yes. The benefit of this approach is dependency injection, but I think this one makes sense to move into the DecisionTaskProcessor. I'll fix it, thanks
| host = configuration.host | ||
| port = configuration.port | ||
|
|
||
| hostname = `hostname` |
There was a problem hiding this comment.
Socket.gethostname avoids invoking another program
There was a problem hiding this comment.
Oh, nice, didn't know of this one. I'll add it as a follow-up PR (we had hostname for a while and I want to keep changes related to config in this PR)
lib/cadence/configuration.rb
Outdated
| ).freeze | ||
| end | ||
|
|
||
| def for_execution |
There was a problem hiding this comment.
This seems to only be used for passing in defaults to ExecutionOptions.new. Should this be named default_execution_options instead?
There was a problem hiding this comment.
That does make sense, yes
antstorm
changed the title
antstorm
added
the
sync pending
DeRauk
added
synced
* Rename Cadence::Client to Cadence::Connection * Move away from global configuration * Ensure correct configuration is passed when generating a poller connection * Rename Configuration#for_execution to Configuration#default_execution_options * Move Connection initialization inside the TaskProcessor classes
Update version to 0.1.7
5 participants
I'm working on a bigger change that will turn Cadence into an initializable class rather than using a single global instance. This will allow connecting your app to multiple clusters (this is handy for easy migrating from one Cadence cluster to another or more exotic solutions such as client-side sharding).
This is the first step which makes configuration local, passed to all classes that are using it