temporal icon indicating copy to clipboard operation
temporal copied to clipboard

Add ServiceRegistry class

Open ychebotarev opened this issue 8 months ago • 6 comments

What changed?

Add ServiceRegistry class. This change:

  • introduce ServiceRegistry class
  • Add it to the Fx
  • add it to shard context

Why?

The goal is to "fill the gaps" for Fx library. ServiceRegistry is a simplified version of "service locator", designed to handle "singletons". It will allow to "extract" instances from ServiceRegistry, while keeping main dependency chains in Fx.

ServiceRegistry will handle such singletons as various host-level caches, 'namespace_registry', etc. It will be easy to substitute them with mocks/stubs, thus simplifying testing. This will allow us to stop using ShardContext (and randomly other classes) as a "property bag", and remove/reduce some dependencies.

Main motivational scenarios: be able to write a code like:

config = serviceregistry.Get[*Config](sr)
wfCache = serviceregistry.Get[*WorkflowCache](sr)
// do something with config in cache

instead of keeping those "config" and "cache" as members of the class initialized during construction

Avoid "passthrough" functions like:

func (a *A)DoSomething(b *B,c *C) {
   a.some_member.DoSomething(b, c)
}

How did you test it?

  • [ ] built
  • [ ] run locally and tested manually
  • [ ] covered by existing tests
  • [X] added new unit test(s)
  • [ ] added new functional test(s)

ychebotarev avatar May 29 '25 02:05 ychebotarev

This seems like too much overlap with fx. If we want the shard context to be less of a bag, why not construct the shard context (and everything owned by it) as a separate fx app itself? There's no rule that fx apps have to be constructed only at process start. That could help clean up the shard context start/stop lifecycle also.

dnr avatar May 29 '25 19:05 dnr

This seems like too much overlap with fx. If we want the shard context to be less of a bag, why not construct the shard context (and everything owned by it) as a separate fx app itself? There's no rule that fx apps have to be constructed only at process start. That could help clean up the shard context start/stop lifecycle also.

I don't see much overlap, the whole point is to fill the gaps that Fx has. Specifically - "give me whatever registered when I need".

I want to be able to write a code like:

config = serviceregistry.Get[*Config](sr)
wfCache = serviceregistry.Get[*WorkflowCache](sr)
// do something with config in cache

instead of keeping those "config" and "cache" as members of the class initialized during construction, or stored at some top level and send "downstream". Because of that we have functions like

func (a *A)DoSomething(b *B,c *C) {
   a.some_member.DoSomething(b, c)
}

I also want to "stub" this is tests. I can't do that right now with Fx, and I don't see how I can do that.

ychebotarev avatar May 29 '25 21:05 ychebotarev

Trying to understand the pros and cons here... Practically, instead of using shardContext as a property bag, this registry will be the new property bag and get passed around everywhere? and what we are getting is essentially a cleaner ShardContext interface?

With today's explicit methods on shardContext, I am able to figure out where a given component is used. But looks like with registry this is harder. And the problem with property bag in general is hard to figure out dependencies of a component. DoSomething(b, c) immediately tells me it depends on b, c to work, while DoSomething(registry) gives 0 information.

I feel ok if we limit the usage only to those commonly used components like namespace registry, entity cache which are pretty much used by all the components in the system. And we need to very strict about any new components we add to it. Other more specialized components should be explicitly listed in the constructor or method signature.

Stepping back a bit, can you give a more specific example of the problem you are trying to address with this?

Also re. using mock in tests. Is it unit test, functional tests, something else? I thought with fx in functional tests you can replace any dependencies with a mock or a different impl. There are examples in tests/dlq_test.go. If it's unit tests, then mock can just be passed into the component constructor as a dependency, not sure how fx is involved.

yycptt avatar May 29 '25 23:05 yycptt

I don't see much overlap, the whole point is to fill the gaps that Fx has. Specifically - "give me whatever registered when I need".

That's really not a gap, though, just a difference in usage pattern. Using fx you just get the things you need when you're constructed and stash a reference to them. It's a little more "static" than asking for them on-demand, but more efficient and (IMO) more clear: all the dependencies are documented in one place.

instead of keeping those "config" and "cache" as members of the class initialized during construction,

But what's wrong with keeping them as members of the class during construction? That has some nice readability benefits.

or stored at some top level and send "downstream". Because of that we have functions like

We have to send them downstream no matter what. We do it either with structs and params (now), or with structs and fx, or with a big map (this proposal). I like structs more than maps, they're more explicit and static, which is good when talking about dependencies.

I also want to "stub" this is tests. I can't do that right now with Fx, and I don't see how I can do that.

You could do it if we constructed the context with fx: you can add new fx options to override the default dependencies from the controller. I think there's plenty of support for this, but I'd have to see the specific code you want to write to be more specific.

dnr avatar May 30 '25 00:05 dnr

I start writing a response. but it becomes too large. I will create a meeting and we will talk.

ychebotarev avatar May 30 '25 02:05 ychebotarev

I am with David here.

alexshtin avatar May 31 '25 07:05 alexshtin

This PR was marked as stale. Please update or close it.

github-actions[bot] avatar Oct 02 '25 00:10 github-actions[bot]