Remove skip_generate_workflow_task from signal APIs #476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cretz
reviewed
Nov 14, 2024
| // Indicates the signal did not generate a new workflow task when received. | ||
| bool skip_generate_workflow_task = 5; | ||
| reserved 5; |
There was a problem hiding this comment.
I do not believe this is a safe/compatible change. We have an obligation to have JSON/field-name compatibility, not just proto binary compatibility, especially for history but also for RPC requests since we have an HTTP API. Can we just document that it's deprecated/ignored?
alexshtin
changed the title
alexshtin
added a commit
to temporalio/temporal
that referenced
this pull request
Nov 15, 2024
## What changed? <!-- Describe what has changed in this PR --> Remove `SkipGenerateWorkflowTask` flag from signal APIs request. Some sort of revert of #4091. ## Why? <!-- Tell your future self why have you made these changes --> This flag breaks important system invariant: Workflow history must end with Workflow Task event OR event that is no-op for SDK (such as `ActivityScheduled` or `TimerStarted`). This invariant will be leverage at #6709 where server must guarantee that it won't ship actual (non command) events to the worker twice. PR to remove flag from APIs: temporalio/api#476. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Existing tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> Although this flag is not exposed by any SDK and not documented, there is a chance that someone use it in production scenarios. For them it will be a breaking change. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> Not documented. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No.
alexshtin
force-pushed
the
feature/remove-skip-wft-flag
branch
2 times, most recently
from
2ce642d to
1abaed7
Compare
alexshtin
force-pushed
the
feature/remove-skip-wft-flag
branch
from
1abaed7 to
d6d0a99
Compare
pdoerner
approved these changes
Nov 15, 2024
cretz
reviewed
Nov 15, 2024
| // Indicates that a new workflow task should not be generated when this signal is received. | ||
| bool skip_generate_workflow_task = 9; | ||
| reserved 9; |
There was a problem hiding this comment.
I think we need to do the same thing here as we did with the history, we need to deprecate/document but not remove the field. These things are part of the HTTP API and need to be JSON compatible.
cretz
approved these changes
Nov 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Remove
skip_generate_workflow_taskflag from signal APIs and events.Why?
See temporalio/temporal#6810 for details.
Breaking changes
Yes, but shouldn't be used.
Server PR
temporalio/temporal#6810 will be merged before this PR.