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

Provide option to exclude all events from being reapplied beyond reset point #454

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

gow
Copy link
Contributor

@gow gow commented Sep 26, 2024

Added a new option RESET_REAPPLY_EXCLUDE_TYPE_ALL that the operator could choose to completely disable reapplying of all events beyond the reset point.

Some events are re-applied (ex: Nexus events) without having an option to opt-out. So we are providing an option to completely exclude all events.

@gow gow requested review from a team as code owners September 26, 2024 19:15
Copy link
Contributor

@dandavison dandavison left a comment

Choose a reason for hiding this comment

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

This makes sense and LGTM. (In fact the CLI already supports this feature; so it can just map its all option to the new enum value when available.)

@gow gow enabled auto-merge (squash) September 26, 2024 19:38
@gow gow disabled auto-merge September 26, 2024 20:47
@alexshtin
Copy link
Member

I would add RESET_REAPPLY_EXCLUDE_TYPE_NEXUS value instead. The property of this enum type is repeated (i.e. array) and it looks odd to me to have RESET_REAPPLY_EXCLUDE_TYPE_ALL in a list which supposed to list types to exclude. "All" semantic can be achieved by explicit listing of all possible values.
If there is a case when events shouldn't be reapplied at all, I believe hardcoding check for all 3 of these is acceptable solution.

@gow gow merged commit 69fecea into master Sep 27, 2024
3 checks passed
@gow gow deleted the cg/reset_event_reapply branch September 27, 2024 00:36
@dandavison
Copy link
Contributor

I agree with @alexshtin that it would be a type error / hack to have an item in this enum with "all" semantics, so if we can avoid that that would be best. However, the behavior of reapplying events after reset may be very surprising to some users: in fact some may consider it to be "incorrect". So I suspect that the CLI and Web UI at least will want to offer a way to disable it without the user having to explicitly disable reapplication of all event types.

@bergundy
Copy link
Member

You should always reapply nexus events, otherwise your operations will get stuck. Let’s discuss @gow. We may need to revert this change.

@dandavison
Copy link
Contributor

dandavison commented Sep 27, 2024

@bergundy presumably we do still need an "escape hatch" allowing people to prevent Nexus events being reapplied for whatever reason (perhaps they're doing some specialized testing, using Nexus in a non-standard manner, etc).

@bergundy
Copy link
Member

The problem is that once an async operation is started, you're waiting for a completion from an external source. If the completion has already been delivered, you'll be in stuck state.

If you're resetting to between scheduled and started you're probably fine but there are subtleties to take into account.

In any case I wouldn't recommend skipping reapply for nexus in general. I would be fine documenting these concerns and giving control to users but my gut says let's not give them a footgun in the first place.

@gow
Copy link
Contributor Author

gow commented Sep 27, 2024

Discussed offline. We decided to keep this option since there is a use-case for operators to be able to reset without re-applying any subsequent events beyond the reset point. However, providing this option would also mean that operators could drive their workflows into a blocked state if they incorrectly choose this option. We decided that we also need additional warning in the CLI/API when this option is chosen (as a followup)

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