Add namespace argument to PerNSWorkerComponent.Register #2847
Conversation
|
If it's too messy to pass nil and ignore it in most of the components, we could split the interface again |
| @@ -276,7 +276,7 @@ func (ws *workerSet) startWorker(wc workercommon.WorkerComponent) (*worker, erro | |||
|
|
|||
| options := wc.DedicatedWorkerOptions() | |||
| sdkworker := sdkworker.New(client, options.TaskQueue, options.Options) | |||
| wc.Register(sdkworker) | |||
| wc.Register(sdkworker, ws.ns) | |||
There was a problem hiding this comment.
Why do you need to pass ns to Register? If you want to keep track of the ns for the perNS worker, you can add ns to your perNS worker struct.
There was a problem hiding this comment.
I want my worker activities to know what namespace they're created for, so they can be sure to only do things in that namespace (this is a double-check since any user in that namespace could send activity tasks to the task queue). The activities struct is created in Register. I couldn't think of any easier way to do this, but if you can let me know.
There was a problem hiding this comment.
Where is that code? Is it not part of this PR?
When you create your WorkerComponent, you know the namespace it is for, so you can put ns as a field of your workerComponentImpl, then in your Register, you know which ns it is for. Isn't that not the case?
There was a problem hiding this comment.
Yes, it's in the next PR. I did this separately to make that one smaller since it's already pretty big.
The WorkerComponent is instantiated by fx and there's one per process, not one per namespace. It's essentially a factory. We could change it to one per namespace, but that doesn't really fix anything: we'd need a WorkerComponentFactory, and the per-ns manager would still need to pass a namespace into some method on the WorkerComponentFactory, so it's the same logic with different names.
There was a problem hiding this comment.
Got it. Now I think it makes more sense to have its own interface. Trying to fit it in the existing interface starts to feel stretched. But I will leave that to you.
There was a problem hiding this comment.
I kinda guessed that would happen eventually :) I split the interfaces again
dnr
changed the title
What changed?
Added
PerNSWorkerComponent, similar toWorkerComponentPass a
*namespace.NamespacetoPerNSWorkerComponent.RegisterWhy?*
It's useful for the per-namespace workers to know what namespace they're assigned to.
How did you test it?
Potential risks
Is hotfix candidate?