Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add normal_task_queue_name to TaskQueue #291

Merged
merged 2 commits into from
May 24, 2023

Conversation

dnr
Copy link
Member

@dnr dnr commented May 24, 2023

What changed?
Include normal task queue name with sticky queues.

Why?
For versioning, we want sticky queues to subscribe to versioning data for their associated normal queues, so that we can properly stop dispatching tasks to out-of-date workers that are still polling. There are various ways to get this information into matching, but the best is to ensure it's always present in the TaskQueue message.

Breaking changes
There are some subtle compatibility concerns around using this field in the server, but in general it won't break anything that already works because by definition, sticky queues are ephemeral, only sticky queues get this new field, and workers using versioning need to be using new versions of the SDK anyway.

@dnr dnr requested review from bergundy and Sushisource May 24, 2023 02:30
@dnr dnr requested review from a team as code owners May 24, 2023 02:31
@@ -45,6 +45,9 @@ message TaskQueue {
string name = 1;
// Default: TASK_QUEUE_KIND_NORMAL.
temporal.api.enums.v1.TaskQueueKind kind = 2;
// Iff kind == TASK_QUEUE_KIND_STICKY, then this field contains the name of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Iff kind == TASK_QUEUE_KIND_STICKY, then this field contains the name of
// If kind == TASK_QUEUE_KIND_STICKY, then this field contains the name of

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure he means if and only if

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that shortcut language is a bit rough here. I would have just dismissed this and assumed we always set this string since it is the normal name after all.

temporal/api/taskqueue/v1/message.proto Outdated Show resolved Hide resolved
Co-authored-by: Chad Retz <chad.retz@gmail.com>
@Sushisource Sushisource merged commit 72e6806 into temporalio:worker-versioning May 24, 2023
Sushisource added a commit that referenced this pull request May 25, 2023

Co-authored-by: Chad Retz <chad.retz@gmail.com>

---------

Co-authored-by: Spencer Judge <sjudge@hey.com>
Co-authored-by: Chad Retz <chad.retz@gmail.com>
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.

3 participants