Process tree structure refactor#149
Conversation
…okerd to adapt to new process tree structure.
| log.info(f'Spawning {brokername} broker daemon') | ||
| tractor_kwargs = getattr(brokermod, '_spawnkwargs', {}) | ||
|
|
||
| # TODO: raise exception when root_nursery == None? |
There was a problem hiding this comment.
if root_nursery == None here, calling this function doesn't make any sense, maybe we could do an if check and raise an exception but not sure which one would be apropiate.
|
|
||
| # init root nursery | ||
| async with tractor.open_nursery() as nursery: | ||
| root_nursery = nursery |
There was a problem hiding this comment.
maybe just put a comment here saying that this assigns the reference to the global module variable so that new calls to pikerd can access this same singleton nursery instance.
| brokername, | ||
| loglevel: Optional[str] = None, | ||
| **tractor_kwargs | ||
| ): |
There was a problem hiding this comment.
Doc string would be good.
Just explain that this func is used to start sub-daemons under pikerd from remote client actors er wtv.
|
|
||
| # TODO: raise exception when root_nursery == None? | ||
| global root_nursery | ||
| await root_nursery.start_actor( |
There was a problem hiding this comment.
Hmm another thing that might be handy here is caching the output portals to each name and then on repeat lookups you just noop if the daemon is already spawned?
| @@ -0,0 +1,79 @@ | |||
| """ | |||
There was a problem hiding this comment.
Add the license header.
We're being pedantic about this since the sw is likely to have a lot of attention in the future.
| ] | ||
|
|
||
| @asynccontextmanager | ||
| async def maybe_spawn_pikerd( |
There was a problem hiding this comment.
Hmm looking at this further I wonder if we should choose a better name like maybe_open_pikerd?
Technically this doesn't actually spawn anything, just might open a nursery.
| yield portal | ||
|
|
||
| else: # no daemon has been spawned yet | ||
| async with maybe_spawn_pikerd( |
There was a problem hiding this comment.
hmm or maybe_start_pikerd()?
| ) as pikerd_portal: | ||
|
|
||
| if pikerd_portal is None: | ||
| await spawn_brokerd( |
There was a problem hiding this comment.
Hmm this is interesting. I wonder if we can avoid having to check for None here (because we're likely going to see this pattern often) and instead hand some kind of local portal thing kinda like what's already inside tractor - but maybe it's over the top?
it would be nice to have the polymorphism to avoid all this branch logic.
goodboy
left a comment
goodboy
left a comment
There was a problem hiding this comment.
Code looks great!
I left a variety of questions, docs suggestions, and a bit of naming changes.
The one thing i wonder if we should consider is some kind of local portal shim to avoid all the branch logic for when we're "spawning" things locally. Not sure i'm convinced it's that big a deal yet tho.
|
@guilledk I made a PR into your branch to update to the new clearing / order systems stuff. I'm not sure if it's more of a hassle to get your branch updated and merge it or I'll just close this PR and make a new one with that updates branch? |
|
Moved to #151. |
2 participants
This started trying to add the
marketstoresupervisor #143, but in the end it turned to a general refactor of the piker process tree.