ResetReapplyExcludeType (preparation for Update reapply) #348
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
dandavison
force-pushed
the
oss-2147-reapply-types-prep
branch
3 times, most recently
from
93058b2 to
324292f
Compare
bergundy
reviewed
Jan 23, 2024
cretz
reviewed
Jan 23, 2024
| temporal.api.enums.v1.ResetReapplyType reset_reapply_type = 6; | ||
| // Event types not to be reapplied | ||
| repeated temporal.api.enums.v1.ResetReapplyExcludeType reset_reapply_exclude_types = 7; |
There was a problem hiding this comment.
Suggested change
| repeated temporal.api.enums.v1.ResetReapplyExcludeType reset_reapply_exclude_types = 7; | |
| repeated temporal.api.enums.v1.ResetReapplyType reset_reapply_types = 7; |
One wonders if this makes more sense, and it can be validated as mutually exclusive with the singular one. I fear we're going to later regret not letting you choose the types to reapply.
I agree that a pure inclusion-based API would be preferable. I'll push a second commit along those lines shortly. |
This reverts commit 91be2e8.
cretz
approved these changes
Jan 23, 2024
We discussed this offline and concluded that exclusion semantics were best seeing as the default needs to be "all". I've included a commit implementing an inclusion variant, but reverted it. |
yycptt
approved these changes
Jan 23, 2024
This was referenced Jan 24, 2024
Closed
dandavison
added a commit
to temporalio/temporal
that referenced
this pull request
Mar 13, 2024
…5360) ## What changed? - Start honoring the new `reset_reapply_exclude_types` request field added in temporalio/api#348 - Change all reapply codepaths to specify excluded event types rather than included - Pass the set of excluded types further down the stack - Delete a pointless helper function ## Why? - We must honor the new request field because it is in the API and it is how we are going to control update reapply when we implement that - The previous code was not future-proof: there were various codepaths passing `enumspb.RESET_REAPPLY_TYPE_SIGNAL` to mean "reapply everything". This PR changes those codepaths to pass an empty set of exclusions, which is robust to future additions of new reappliable event types. - The actual work is done by `reapplyEvents`, and the excluded types had to be pushed all the way down the stack to this function. The previous code was pretty fragile: it checked that the caller had requested reapplication of signals and then let the downstream code reapply everything. ## How did you test it? - Unit tests ## Potential risks - It could break replication, and reset ## Is hotfix candidate? - No
stephanos
pushed a commit
to stephanos/temporal
that referenced
this pull request
Mar 21, 2024
…emporalio#5360) ## What changed? - Start honoring the new `reset_reapply_exclude_types` request field added in temporalio/api#348 - Change all reapply codepaths to specify excluded event types rather than included - Pass the set of excluded types further down the stack - Delete a pointless helper function ## Why? - We must honor the new request field because it is in the API and it is how we are going to control update reapply when we implement that - The previous code was not future-proof: there were various codepaths passing `enumspb.RESET_REAPPLY_TYPE_SIGNAL` to mean "reapply everything". This PR changes those codepaths to pass an empty set of exclusions, which is robust to future additions of new reappliable event types. - The actual work is done by `reapplyEvents`, and the excluded types had to be pushed all the way down the stack to this function. The previous code was pretty fragile: it checked that the caller had requested reapplication of signals and then let the downstream code reapply everything. ## How did you test it? - Unit tests ## Potential risks - It could break replication, and reset ## Is hotfix candidate? - No
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.
These are proposed proto changes in preparation for Update reapply, based on a design by @yycptt.
Currently, the Reset request allows a user to send
RESET_REAPPLY_TYPE_SIGNALorRESET_REAPPLY_TYPE_NONE. The server defaults toRESET_REAPPLY_TYPE_SIGNAL.We are about to start reapplying Updates and, in the future, we may reapply other events such as Nexus-related events.
We do not want to expand the
ResetReapplyTypeenum to encode arbitrary subsets of event types to be reapplied.Instead, this PR adds to the Reset request the ability to send a list of event types to be excluded from reapply.
I have also added one final value to the legacy enum:
RESET_REAPPLY_TYPE_ALL_ELIGIBLE. I propose to make this the new default forResetReapplyType. In practice, today, that has the same consequences (i.e. "reapply signals"), just under a different name.Other than that final addition,
ResetReapplyTypenow becomes deprecated.Thus what I'm proposing is that, in the future, the server's implementation will be to:
ResetReapplyTypeResetReapplyExcludeTypeeventsThe result is the set of event types that will be reapplied during Reset.