Feat: temporal worker#166
Conversation
|
@Baldinof , I'd like to review this |
|
Thanks! Please, go ahead |
|
Hi. Any news? I want to use temporal with this bundle |
| ->defaultValue([ | ||
| NullConverter::class, | ||
| BinaryConverter::class, | ||
| ProtoJsonConverter::class, | ||
| JsonConverter::class, | ||
| ])->scalarPrototype()->end() | ||
| ->end() |
There was a problem hiding this comment.
I don't really like this approach of defining data_converters as array.
Consider if somebody would like to develop their own library, and would like to define one more data converted.
Would they be able to do it?
There was a problem hiding this comment.
I changed it. now it uses tagged services ('temporal.data_converter') - apps using that bundle could just tag their own implementations. the default converters I in the extension - apps could deactivate/remove those?
| use DoctrineORMTraits; | ||
|
|
||
| public function __construct(ManagerRegistry $managerRegistry, ContainerInterface $container, EventDispatcherInterface $eventDispatcher, LoggerInterface $logger) |
There was a problem hiding this comment.
Composition would be better than "traiting"
| namespace Baldinof\RoadRunnerBundle\Temporal\Attributes; | ||
|
|
||
| #[\Attribute(\Attribute::TARGET_CLASS)] | ||
| class AssignToWorker |
| $this->dependencies->getEventDispatcher()->dispatch(new WorkerKernelRebootedEvent()); | ||
| } |
There was a problem hiding this comment.
Probably service reset should be taken into account as well
} elseif ($this->kernel->getContainer()->has('services_resetter')) {
/** @var ResetInterface $resetter */
$resetter = $this->kernel->getContainer()->get('services_resetter');
$resetter->reset();
}| $workerOptions = $workerOptions->withMaxConcurrentActivityExecutionSize((int) $options['max_concurrent_activity_execution_size']); | ||
| } | ||
|
|
||
| if (\array_key_exists('worker_activities_per_second', $options)) { |
There was a problem hiding this comment.
why are all these key array_key_exists checks necessary? all of these configurations have the default value, so what's the purpose of checking it all?
| $container->register("temporal.client.{$name}.factory", WorkflowClientFactory::class) | ||
| ->setArguments([ | ||
| '$serviceClient' => new Reference("temporal.client.{$name}.service_client"), | ||
| '$dataConverter' => new Reference('temporal.data_converter'), |
There was a problem hiding this comment.
it would also make sense to have data converter configurable per client
There was a problem hiding this comment.
I abstained from changing that for now - if it's really useful, we could add it later. for now, I finally wanted to integrate all your remarks. :) thanks again for your review :)
|
@crazy-weasel , it seems that |
|
Hi @rela589n thanks for the review :) I can't give you an explanation on all choices, as I just expanded on someones code, but I try to if an explanation is necessary and/or incorporate your changes / make the code better. |
|
Hi, I just want to let you all know, that I did not forget about the review. Unfortunately, I got sick and recovery took some time.. I'll get to it soon, after I finish some work related backlog :) Thanks for your patience! |
It's alright, take care! I did not had much time for this repo lately too :( |
|
I have been following this PR with some interest as I am working on a Temporal project, and have thus been evaluating this bundle with this feature. I ran however into a bug in the latest commit of this PR, whenever I am trying to execute an Activity, I run into a ReflectionException: This error went away when I went to a commit earlier ( It seems introducing the activity factories introduced a bug which is tripping up the Dispatcher reflection logic. |
|
hi! any updates? |
|
Any updates? |
|
Guys, you are the driving force of updates 🙂 |
renames ClientFactory to WorkflowClientFactory adds ScheduleClientFactory
fixes typo removes redundant code
- adding two default interceptors for using the configured reboot-strategie and for Doctrine ORM - registering activities with a factory that fetches the activities from the DI container. that way, new activity-instances are created when the kernel was rebooted. - dispatching events similar to the other workers - OnExceptionRebootStrategy now listens to the WorkerExceptionEvent
add tests change dataconverters from configured as an array, to tagged services
crazy-weasel
force-pushed
the
feat/implement-temporal
branch
from
a71542a to
5f17af3
Compare
|
hi everyone :) sorry for the long wait, but I finally took the time to implement (almost) all of @rela589n suggestions. the data converters are still only configured once for all client. I guess that's good enough for now? I guess tomorrow I'll have an additional look into the debug-commands, and also to adding a data-collector for the symfony profiler. at least for displaying information about started workflows within a request. |
…ns and basic information about the workflow configuration
…ion that is used by the CollectingClientInterceptor
8 participants
This is based on #147
Changes made:
Some of my thoughts can be read in the original PR, starting here: #147 (comment)
Things that still need to be changed (but I'd like some feedback on):
I'd like to use the bundle and this feature in an upcoming project, so I'm very motivated to bring it up to your standards and your feedback is highly appreciated.