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 capabilities field to RespondWorkflowTaskCompletedResponse #458

Merged
merged 7 commits into from
Oct 15, 2024

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Add capabilities field to RespondWorkflowTaskCompletedResponse. Add may_discard_speculative_workflowtask_with_command_events that will allow the SDK to signal to the Server that it supports discarding speculative workflow tasks with command events. When this is set to true it will allow the server to discard a lot more speculative workflow tasks then it can today.

//
// (-- api-linter: core::0140::prepositions=disabled
// aip.dev/not-precedent: "with" used to describe the workflow task. --)
bool discard_speculative_workflow_task_with_command_events = 1;
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
bool discard_speculative_workflow_task_with_command_events = 1;
bool discard_speculative_workflow_task_with_events = 1;

command_events is not a common well known name. And in fact, all events falls in this category. Let's say we enable signal without WFT one day.

Copy link
Member

@Sushisource Sushisource Oct 10, 2024

Choose a reason for hiding this comment

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

We use "command_events" in the SDKs all over the place to refer to the events corresponding to commands. So, not all events are that - like signaled events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah command_events is a common term in the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that being said I am not opposed to removing it

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Are there any SDKs with this capability today?

@Quinn-With-Two-Ns
Copy link
Contributor Author

Are there any SDKs with this capability today?

Yes, all latest SDK now do. but old versions didn't

@bergundy
Copy link
Member

Are there any SDKs with this capability today?

Yes, all latest SDK now do. but old versions didn't

I would maybe consider hard coding these SDK versions on the server instead of adding this mechanism since it's easy enough for us to track those but I'm okay adding it if people see this useful beyond the single use case.

@cretz
Copy link
Member

cretz commented Oct 15, 2024

Yes, all latest SDK now do. but old versions didn't

@Quinn-With-Two-Ns - what happens to older SDKs if the server were to just behave as if this new field was always true? Are all SDKs that don't work with this part of update pre-release instead of public preview? Just trying to help justify a new permanent proto field just for pre-release update SDKs.

@Quinn-With-Two-Ns
Copy link
Contributor Author

what happens to older SDKs if the server were to just behave as if this new field was always true?

The history will be corrupted , we have already had multiple meetings and a design doc to converage on this solution

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 277bced into temporalio:master Oct 15, 2024
3 checks passed
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.

5 participants