Add Temporal support #49
Conversation
|
Hi! Thank you for the great work, and sorry for the late review! I like the autoconfiguration of workflows / activities :) |
| HttpWorker $httpWorker, | ||
| TemporalWorker $temporalWorker |
There was a problem hiding this comment.
Doing that forces the DIC to resolve the 2 workers but only one is used (and Temporal might even be not installed).
It's the kind of place where we could inject the container and use return $container->get(HttpWorkerInferface::class) so the service is created only if needed.
There was a problem hiding this comment.
I've marked them with lazy()
There was a problem hiding this comment.
Marking them lazy force the users to install symfony/proxy-manager-bridge.
If you feel incomfortable injecting the whole container, maybe we could use a service locator to inject a scoped container, what do you think?
There was a problem hiding this comment.
I don't like service locator. As you can see :)
What do you think if I add supports($mode): bool method into workers and configure resolver to get array of workers and pass the conditions below into them?
It will look like the following:
TemporalWorker:
supports($mode):
$mode === 'temporal'
HttpWorker:
supports($mode):
$mode === 'http'
And this resolver will look:
class WorkerResolver
{
function __construct(iterable $workers){}
function resolve($mode) {
foreach($this->workers as $worker)) {
if($worker->supports($mode){
return $worker;
}
throw new Exception();
}WorkerResolver will be configured in the container with a tag and !tagged_iterator.
There was a problem hiding this comment.
If you do this, the DIC may still instantiate the 2 workers but only one is useful, I think a locator is the best way, it's precisily why service locators exists, in the Symfony doc:
Sometimes, a service needs access to several other services without being sure that all of them will actually be used
|
Hi, I will look at your comments at the weekends or a bit later. |
|
@Baldinof I've resolved your comments. Can you please review again?
Like this format: temporal_workers:
first_worker:
option1: value1
option2: value2:
second_worker:
option1: value3
option2: value4This settings will be converted and used in the factory that mentioned above. |
|
@Baldinof can you have a look? |
|
Yes! Sorry for being late, I will try to have a look by the end of this week!
|
| HttpWorker $httpWorker, | ||
| TemporalWorker $temporalWorker |
There was a problem hiding this comment.
Marking them lazy force the users to install symfony/proxy-manager-bridge.
If you feel incomfortable injecting the whole container, maybe we could use a service locator to inject a scoped container, what do you think?
| service(TemporalWorker::class), | ||
| ]); | ||
|
|
||
| $services |
|
|
||
| namespace Baldinof\RoadRunnerBundle\Worker; | ||
|
|
||
| interface WorkerResolverInterface |
There was a problem hiding this comment.
This should also be used in Baldinof\RoadRunnerBundle\Runtime\Runner
|
@rmikalkenas if you mind you may finalize this PR by yourself. I lost all context of this PR and don't have time to reproduce all scenarios |
| * @internal | ||
| */ | ||
| final class Worker implements WorkerInterface | ||
| class HttpWorker implements WorkerInterface |
5 participants
[#48] Added https://github.com/temporalio/sdk-php support.
If you want also tests I can add them a bit later or you can do it yourself 😉 .
Now I want to discuss about solution in general.
You can see how it works in https://github.com/xepozz/symfony-temporal-demo.