Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Temporal support #49

Open
wants to merge 16 commits into
base: 2.x
Choose a base branch
from
Open

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Jul 8, 2021

[#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.

@xepozz xepozz mentioned this pull request Jul 8, 2021
@Baldinof
Copy link
Owner

Hi!

Thank you for the great work, and sorry for the late review!

I like the autoconfiguration of workflows / activities :)

composer.json Outdated Show resolved Hide resolved
config/services.php Outdated Show resolved Hide resolved
config/services.php Outdated Show resolved Hide resolved
src/Temporal/TemporalWorkerFactory.php Outdated Show resolved Hide resolved
src/Worker/TemporalWorker.php Show resolved Hide resolved
Comment on lines +17 to +18
HttpWorker $httpWorker,
TemporalWorker $temporalWorker
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've marked them with lazy()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/Worker/WorkerResolver.php Outdated Show resolved Hide resolved
src/Command/WorkerCommand.php Show resolved Hide resolved
@xepozz
Copy link
Contributor Author

xepozz commented Jul 21, 2021

Hi, I will look at your comments at the weekends or a bit later.

@xepozz
Copy link
Contributor Author

xepozz commented Aug 1, 2021

@Baldinof I've resolved your comments. Can you please review again?
Also I have an idea how to configure workers are creating in https://github.com/xepozz/roadrunner-bundle/blob/a239d1e6c05dbd78564dcdd72d55ad87d7f78f9a/src/Worker/TemporalWorker.php#L41-L41
We can use usual config file to configure:

  1. Amount of workers
  2. Their options (https://github.com/temporalio/sdk-php/blob/master/src/Worker/WorkerOptions.php#L23)

Like this format:

temporal_workers:
  first_worker: 
    option1: value1
    option2: value2:
  second_worker: 
    option1: value3
    option2: value4

This settings will be converted and used in the factory that mentioned above.
I will create issue later if we finish this PR.

@xepozz
Copy link
Contributor Author

xepozz commented Aug 11, 2021

@Baldinof can you have a look?

@xepozz xepozz requested a review from Baldinof August 11, 2021 19:53
@Baldinof
Copy link
Owner

Baldinof commented Aug 11, 2021 via email

Comment on lines +17 to +18
HttpWorker $httpWorker,
TemporalWorker $temporalWorker
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/Command/WorkerCommand.php Show resolved Hide resolved
service(TemporalWorker::class),
]);

$services
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in services.php


namespace Baldinof\RoadRunnerBundle\Worker;

interface WorkerResolverInterface
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be used in Baldinof\RoadRunnerBundle\Runtime\Runner

@rmikalkenas
Copy link
Contributor

@Baldinof @xepozz any plans to finalize this PR and add temporal support?

@xepozz
Copy link
Contributor Author

xepozz commented Aug 30, 2024

@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

@praswicaksono
Copy link

@xepozz @Baldinof I take over this integration please review to new PR: #147 any review will be appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants