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.
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
Feature: Add support for running migration in batches #7755
base: epic/campaigns
Are you sure you want to change the base?
Feature: Add support for running migration in batches #7755
Changes from 33 commits
5bb5d6d
6ee4163
783e27f
da54e5f
38d040e
9d498ab
27d6d0d
35a7290
f79a040
3311632
cbdc32d
761e21d
9d07d6a
20994e0
ea5662e
2c3fa32
10afdac
698f631
5ff0d21
a7c2c42
990bbf9
972e7fd
b7d31e3
b100127
58396d9
16b6af6
b10cfa3
50b8832
8a974b9
c620062
070b23e
069d3e2
d5ddbc5
dc5bf12
6698d31
1e3634f
2721987
be3c08a
475d608
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of notes/questions on this:
Namespace
The new RESTful routes in GiveWP are going to use the
givewp/v3
namespace, and I believe exist in aV3
sub-domain — double-check with @jonwaldstein on thisStarting / Restarting Migrations
The concept of rescheduling failed actions is interesting. If I recall correctly, the system works such that only a single migration can actually be failed at a time, which then prevents subsequent migrations from running. So resuming migrations really means restarting the failed migration, and once it's successful then the subsequent migrations will follow. So they're not "failed", they're simply blocked.
This makes me wonder if we really need to introduce the concept of "failed" to the API, or if it's simply sufficient to say that a given migration can be "started". If it's an automatic migration then this is usually unnecessary, but can be triggered in the admin if necessary. For a batch migration, this may be a more necessary step — and it may not really matter whether the migration has yet to run or failed to run and needs to be attempted again.
RESTful API
This makes me think we could actually support a PUT/PATCH for the migration resource:
This makes it RESTful, and would even allow for JS Entities for interacting with the migration. Status would be a simple property of the resource, and the admin can change it to only this one status — wherein we also check that the existing status is in a state that's able to be switched to run (as opposed to try to run a running migration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about our migration system or AS?
This is true for our migration system, but not for AS.
The thing with Action Scheduler is that it doesn't care about failed/canceled actions. AS runs actions concurrently so there can be many failed/canceled actions at the same time, but that doesn't stop AS from running the rest. Once an action is failed or canceled, AS will not try to run it again. Actually, it will delete all of them after some time. Also, devs behind AS are recommending if the action fails, but you need that action to run, you should reschedule it.
I believe we can't really leave any orphaned data like in this example - Assign donations to a campaign. That's why I introduced this. Also, if batch migration has failed/canceled actions, it's not marked as failed, it is marked as incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about the migration system. I guess we do need to think about what happens if batch migration enters into a partial state — wherein some of the steps have succeeded and others have failed. 🤔
To be clear, at no point should two migrations be running — even if they're batch migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is not happening. Not even in our current broken system 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to hop on a call, at a time that isn't interrupting work, and think through this a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be useful to add another method or a class constant:
In the not too distant future we'll have migrations that we want to prompt the user to begin. Those migrations should set this to false. All it does is that if the
MigrationsRunner
encounters this as false, then it simply stops and doesn't proceed.No need to build a UI or anything for this now, as it's outside of the scope of what you're doing. But it came to mind as something we'll want in this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I say all that, maybe it's best to not introduce this now and instead introduce it when we build the UI, later... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a simple change, but yeah, we can add that later.