Skip to content

Conversation

yuandrew
Copy link
Contributor

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?
use worker_set_key instead of runtime_key for worker command nexus task queue

Why?
There can be multiple clients (i.e. with different auths) within the same runtime/namespace, so we want a more specific category for these nexus worker command requests

Breaking changes
No, WIP feature

Server PR

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Love this, thanks for being willing to revisit this!

Since we don't reference this field anywhere, this should be a safe change from a compatibility POV

"workerSetKey": {
"type": "string",
"title": "Worker process identifier. This id should be unique for all _processes_\nrunning workers in the namespace, and should be shared by all workers\nin the same process.\nThis will be used to build the worker command nexus task queue name:\n\"temporal-sys/worker-commands/{process_key}\""
"title": "Worker process identifier. This id should be unique for all worker sets, or\nclients, running workers in the namespace, and should be shared by all\nworkers in the same worker set.\nThis will be used to build the worker command nexus task queue name:\n\"temporal-sys/worker-commands/{worker_set_key}\""

Choose a reason for hiding this comment

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

Why does this say worker process identifier?

Also, please help me understand the following. I am not able to correlate the PR description with this comment. For example, the description talks about runtime key and how clients with different auth need to be categorized. But here, it sounds like all workers running on the same host will have the id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout, I should update this as well to Worker Set Identifier. We previously used a worker process identifier each separate process. But I later discovered we can have multiple clients with different auth settings (i.e. self host & cloud) within the same process, so we need a more granular key.

In the scenario where we have 2 different clients being used in the same process, we would need 2 separate nexus task queues to handle heartbeating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of "runtime", we were previously using that synonymous to process, but that's also technically not true SDK side, not sure why anyone would want to have multiple runtimes in the same process, but they technically could, I think

Choose a reason for hiding this comment

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

Is this an already agreed upon name, or are we open to suggestions ? 😄
If so, I would suggest calling it as a workerGroupingKey. I feel the intent is more direct by calling it a grouping-key than a set-key.

  • A host can have multiple processes.
  • Each process can have 1 or more workers.
  • Workers within a process can be grouped together using a grouping key.
    IIUC, the SDK would construct this automatically based on the worker attributes that the user has defined (example, auth settings).

Question: How is this key generated?

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