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

DropFlow: remove context deadline in shutdown flow #2292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Amogh-Bharadwaj
Copy link
Contributor

Not sure why we are setting a context deadline of 5 minutes for drop flow because after 5 minutes we are anyways performing handleCancelWorkflow in the event of drop flow taking a long time (to drop the slot etc)

Currently the context is getting cancelled after 5 minutes, taking us to the first case:

	case err := <-errChan:
		if err != nil {
			slog.Error(fmt.Sprintf("unable to cancel PeerFlow workflow: %s. Attempting to terminate.", err.Error()))
			terminationReason := fmt.Sprintf("workflow %s did not cancel in time.", workflowID)
			if err := h.temporalClient.TerminateWorkflow(ctx, workflowID, runID, terminationReason); err != nil {
				return fmt.Errorf("unable to terminate PeerFlow workflow: %w", err)
			}
		}

and erroring out

@serprex serprex requested a review from heavycrystal November 26, 2024 04:59
@heavycrystal
Copy link
Contributor

heavycrystal commented Nov 26, 2024

nit: the workflow itself can run indefinitely, it's only the .Get call that gets cancelCtx so it waits 5 minutes for the result basically

I agree the select branches for DropFlow seem redundant (the cancel branch that we probably wanted will almost never be taken); but the core decision to take here is whether we want DropFlow to run indefinitely or not.

If we want to timebox DropFlow we can remove the time.After branch and have errChan branch cancel the workflow; if we want it to run indefinitely we don't cancel the workflow and return no error

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