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

Cascading delays #70

Open
ryanhiebert opened this issue Jul 26, 2024 · 4 comments
Open

Cascading delays #70

ryanhiebert opened this issue Jul 26, 2024 · 4 comments

Comments

@ryanhiebert
Copy link
Owner

ryanhiebert commented Jul 26, 2024

I'm curious what your reasoning is for having delays collapse instead of cascade.

class A(Migration):
    safe = Safe.after_deploy(delay=timedelta(days=2))

class B(Migration):
    safe = Safe.after_deploy(delay=timedelta(days=1))
    dependencies = [B]  # note: this is not real syntax

In this example, the current implementation would combine the delays to a total of 2 days when it was deployed, but I can see a pretty good argument that a more intuitive behavior would be for delays to cascade through their dependencies instead to a total of 3 days, giving an opportunity at each stage to identify problems.

Because we have, as a library, checked out of doing the unsafe / after migrations historically, our documented mode of operation has practically been to collapse them, but I do think that I've occasionally manually cascaded them.

With the hack I described in #68 I've also effectively done a cascade by blocking the follow-up migration from happening until the earlier one has completed, but it is a stronger form of a cascade that intentionally causes problems if merged, even if the codebase is already compatible with the migrations from the start.

If I try to expand to other domains to help get an intuition for what might be expected or confusing, I know that collapsing margins, and the rules for when they cascade instead, are one of the most confusing features of CSS.

I suspect that the reason for the implemented choice was implementation simplicity. Is that correct, and do you have a strong sense of when a cascading delay might typically be expected or preferred, as I'm coming to suspect?

@tim-schilling
Copy link
Collaborator

Having them cascade never crossed my mind.

In most cases, the apps I work on there's only one change going out at a time. Having multiple long running operations (waiting to see how a new change sits) isn't something that I've come across often. Or at least not often enough that it would come to mind.

Most of my work still remains deploying a singular change and seeing that through before moving onto the next. Aside, having the apps split up well makes this easy.

@tim-schilling
Copy link
Collaborator

This may be a challenge to nail down and communicate properly.

  1. When does th delay start being counted against if there are multiple dependencies with a delay? I'd suspect we'd go with the latest.
  2. What if one of the listed dependencies has a subsequent migration with another delay? I suspect we'd ignore it.
  3. Will we support run_before? And what if a new migration comes in and technically should delay it?

@ryanhiebert
Copy link
Owner Author

ryanhiebert commented Jul 26, 2024

  1. A delay wouldn't begin until all of their dependencies have completed and another run of safemigrate happens. This would allow delays to change in the parent migration and for those changes to be respected until the point that the delay began for the previous delay.
  2. I think you're referring to sibling delays, rather than chained delays. Yes, we'd only delay based on dependencies.
  3. Yes, we should support run_before the same as regular dependencies, just as we do for blocking migrations. A new migration coming in is an edge case I hadn't thought of yet. Based on my answer for point 1, I think the consistent semantic would indeed be to re-up the starting delay. It's enough of an edge case that I'd call that a backward-compatible bugfix if it wasn't how we implemented it at first, though.

@ryanhiebert
Copy link
Owner Author

I'd prefer this semantic over collapsing if we were to only choose one, so I'd want this to be the default. If you felt open to a patch, I'd feel motivated to do it right away to see if I could avoid a breaking release. If the existing semantic was released, I wouldn't be in any particular rush to implement this.

I don't think the breaking change would be very significant in this case (since as you've described, I expect most users to not run into this case regularly), so I'm not overly worried about it, but avoiding breaking changes where possible does motivate me.

If we had this semantic, I don't think I'd have any immediate motivation to add the collapsing semantic as an alternative, though I'd probably open a ticket to track the idea, because I can see some utility for it. I prefer collapsing be the special-case; I see a pretty nice analog to elidable migrations and operations, for example.

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

No branches or pull requests

2 participants