Conversation
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
|
@Baldinof , I'd like to review this |
|
Thanks! Please, go ahead |
|
Hi. Any news? I want to use temporal with this bundle |
| "temporal/sdk": "^2.10" | ||
| }, |
There was a problem hiding this comment.
is there any particular reason for not using ^2.0?
| } | ||
|
|
||
| if (class_exists(WorkflowInterface::class)) { | ||
| $services->set('temporal.exception_interceptor', ExceptionInterceptor::class) |
There was a problem hiding this comment.
shouldn't ExceptionInterceptorInterface be used instead?
| $services->set('temporal.exception_interceptor', ExceptionInterceptor::class) | |
| $services->set('temporal.exception_interceptor', ExceptionInterceptorInterface::class) |
| $container->addCompilerPass(new InterceptorCompilerPass()); | ||
| } | ||
|
|
||
| if (interface_exists(WorkflowClientInterface::class) && class_exists(WorkflowInterface::class)) { |
There was a problem hiding this comment.
I think one check would be enough
It shouldn't be possible that one of these names exists, while the other doesn't
| ->addTag('baldinof.roadrunner.grpc_service'); | ||
| } | ||
|
|
||
| if (interface_exists(WorkflowClientInterface::class) && class_exists(WorkflowInterface::class)) { |
There was a problem hiding this comment.
I guess one check should be enough (WorkflowInterface)
| '', | ||
| [ // Symfony overrides the first argument with the DSN, so we pass an empty string |
There was a problem hiding this comment.
| '', | |
| [ // Symfony overrides the first argument with the DSN, so we pass an empty string | |
| '', // Symfony overrides the first argument with the DSN, so we pass an empty string | |
| [ |
| ->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?
| 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
|
@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 🙂 |
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.