[Feature Request] Add a global worker registry

[antlai-temporal]

Is your feature request related to a problem? Please describe.

In order to enable Eager Workflow Start we need to find compatible workers that are associated with the current core instance. This feature will provide an internal interface that given a (namespace, task_queue_name) pair provides a list of suitable workers.

Describe the solution you'd like

WorkerRegistry is a singleton object that is lazily initialized and protected by a RwLock. Workers register at creation time (Worker.new_with_pollers()) and unregister during shutdown (Worker.initiate_shutdown()).

The main consumer of the query interface is the client, and since we don't want the client crate to depend on other internal crates,
we use dependency injection with a trait defined by the client.

Additional context

This is just a stepping stone towards enabling Eager Workflow Start for core-based languages

[cretz]

global [...] WorkerRegistry is a singleton object that is lazily initialized and protected by a RwLock.

Disagree. The client can just keep a collection of weak references to known workers that are associated with it. Can be a client-crate-defined trait like EagerWorker that exposes calls the worker would implement. Eager workflow start is only a per client thing, not a process-wide one.

[antlai-temporal]

Disagree. The client can just keep a collection of weak references to known workers that are associated with it.

What this is trying to do is to help the client find workers that are "associated with it", in the sense that they target the same queue+namespace.
There is no reason why the local worker should use the same client connection, this is a needed restriction for Go and Java because otherwise there is no shared state between them. And this flexibility makes it easier to make it transparent for the Core SDKs, otherwise it is confusing that we have to configure them to use the same client to enable it...

Can be a client-crate-defined trait like EagerWorker that exposes calls the worker would implement. Eager workflow start is only a per client thing, not a process-wide one.

Internally this is how Eager Workflow Start will work, it just that this client trait is stateless, and just finds all the relevant workers using the registry injected by Core with the first "start workflow". All this happens within core abstractions, the SDKs clients are not modified...

[cretz]

this is a needed restriction for Go and Java because otherwise there is no shared state between them. And this flexibility makes it easier to make it transparent for the Core SDKs, otherwise it is confusing that we have to configure them to use the same client to enable it

Technically Go or Java could have had globals for this info too. They choose to make this client level state. It's not confusing that eager workflow start is per client, and it would make hardly any sense to create multiple clients for different workers of the same task queue and need to start workflows on each others' workers. Keeping the client-worker relationship works well in all SDKs, not just Go/Java.

My only objection is globals. I don't think we want a process-wide global in Go, Java, or core for a client knowing which workers it can use.

[antlai-temporal]

This is similar to metrics in Core, it also uses lazy static to maintain some state accessible.

I think keeping track of which workers are available, and what tasks they can process, should be a Core-level abstraction and not a client abstraction. Sooner or later we will need this in other contexts, for example, when flexing workers. And I agree that directly exposing a global in Java or Go SDKs is a hack, but here it is all internal to Core, not directly visible to application code...

Another reason is Typescript, where the workers never share a connection with the client because they use a different protobuf implementation. In theory, we could still bridge somehow (with a hook) if the state is maintained by core but not if it is only in the client.

@Sushisource comments?

[antlai-temporal]

And just to make clear what I mean by "global", it is not going to be a Rust "naked global" visible everywhere, I will wrap it in a struct, and then the new() will define a static map within the local scope, only visible to methods of this struct.

Similar to default_resource_kvs() in https://github.com/temporalio/sdk-core/blob/master/core/src/telemetry/mod.rs#L246-L249

[cretz]

This is similar to metrics in Core, it also uses lazy static to maintain some state accessible.

Not mutable state. Global mutable state is bad and unnecessary.

I think keeping track of which workers are available, and what tasks they can process, should be a Core-level abstraction and not a client abstraction.

It should be "core-client level"

directly exposing a global in Java or Go SDKs is a hack

Not about exposing, even having mutable global state is a hack/bad

Another reason is Typescript, where the workers never share a connection with the client because they use a different protobuf implementation

Then there needs to be an in-TS way to associate workers with TS clients and push eager tasks. Today TS is the only SDK whose client is different for workflow and worker work.

[antlai-temporal]

Let me clarify what is "global mutable state" in this context.
I'm creating a singleton service that internally has mutable state protected by locks and exposes a narrow, and safe, interface to modify that state.
If we had a dependency injection mechanism in core we wouldn't have this discussion, exactly the same singleton service would be created at start time, and "magically" appear where it is needed.
AFAIK, Rust is no friend of fancy pants dependency injection, so the alternative is to create a placeholder for the service at compile time, i.e., static, and initialize it with first use. And to make it available where it is needed we have a global factory method that returns that instance.
So this is not the wild west of global mutable state...

[Sushisource]

I think global is fine. As long as the map holds Weak refs the concerns about lifetimes aren't a problem. Rust makes concerns about improper access patterns a non-issue. Clients may not always be explicitly associated with the worker in situations where you might still like this to work. That's admittedly rare, but a small plus to using a static map.

I'd also be totally fine with storing it in the client, but I think either path is reasonable and whatever is simpler to implement makes sense to me.

[cretz]

We may have to explain that eager on some SDKs works when you reuse the client and on others is process wide. I'm not really concerned about access. It would have been plenty easy to have private static final ConcurrentMap<TargetNamespaceTaskQueueTuple, TemporalWorker> WORKERS = new MapMaker().weakValues().makeMap(); in Java too, but I think it's clearer to users to reuse at a client level instead of process level, but meh.

[cretz]

I realized another reason why we should discourage globals if we can. Core is currently multi-tenant and isolated today. There has been talk of a "core remote" where you can have a general core that supports multiple different remote workers. Similarly with WASM, there is an overhead of starting a WASM instance, so one may want to reuse the same WASM instance but with completely different runtime. It is very normal for core to be used with different runtimes and different thread pools (often to isolate telemetry).

In general, isolated core is a good thing. If you must share this map across clients, can we at least put it on the runtime?

[Sushisource]

That's reasonable - putting it on the runtime would be nice but clients do not know that the runtime exists right now or have access to it (since it's defined in core and the client crate doesn't depend on that, and won't).

I don't know that we'll ever actually do a remote core, but the WASM concern is real. Given that users are indeed kinda doing a dumb thing if they have multiple clients for the same endpoint, then I'm fine giving that up and saying the map exists on the client instance and is populated when the client is passed into a worker.

[antlai-temporal]

Yes, if we want to reuse WASM instances with different runtimes then we cannot have a worker registry per process, and we need to attach state to the client.
I saw that init_worker() takes a client of type AnyClient "to allow passing different kinds of clients into things that want to be flexible". Are there client types that I need to ignore? What is the cleanest way to unwrap the original client?

[antlai-temporal]

It was decided that we will not have a global registry, and instead associate state directly to clients, similar to the Go and Java SDKs