bulkio: Make incremental scheduled backup wait for full backup. #52836
Conversation
|
This change is |
miretskiy
force-pushed
the
dep_schedule
branch
3 times, most recently
from
d0f863b to
0b6e01b
Compare
|
@dt -- rebased on master; ready for review |
miretskiy
force-pushed
the
dep_schedule
branch
from
0b6e01b to
bbe70ee
Compare
There was a problem hiding this comment.
Hm. I donno, this might just be me losing my marbles after staring at too much code, so feel free to just say so, but on first read, I'm about dubious of introducing new concepts like "ScheduleGroup" and "GroupAction". Again, might be just me, but I find the it is a bit harder to grok with the new concepts. Or, alternatively, I expect them to be actual rows in a table called schedule_groups and then to have a group_id column in schedules or whatever. I also don't know that GroupAction is really general to all schedule types?
Thus, my counterproposal would be:
- the backup specific
ScheduledBackupExecutionArgsgains a "optional int64 unpause_on_success` field - the success hook for backups looks at it and, if it is set: a) unpauses the schedule with that ID b) clears it from itself.
WDYT?
pkg/ccl/backupccl/backup_job.go
Outdated
| ctx, | ||
| "lookup-schedule-info", | ||
| txn, | ||
| sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, |
| if err != nil { | ||
| return err | ||
| } | ||
| inc.Pause("Waiting for FULL") |
There was a problem hiding this comment.
"Waiting for initial backup to complete"
There was a problem hiding this comment.
Sure -- that makes sense. I had something like this going -- notify success is a bit tricky w/ jobs; and initially, I was having issues w/ doing it in the resumer and kinda got on a path of doing it through "meta" proto... Eventually, backup resumer on success started working fine - so making these changes backup specific is fine by me.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt)
pkg/ccl/backupccl/backup_job.go, line 590 at r1 (raw file):
Previously, dt (David Taylor) wrote…
this should probably be
NodeUser
Done.
pkg/ccl/backupccl/create_scheduled_backup.go, line 341 at r1 (raw file):
Previously, dt (David Taylor) wrote…
"Waiting for initial backup to complete"
Done.
miretskiy
force-pushed
the
dep_schedule
branch
from
bbe70ee to
b8125ec
Compare
|
Argh... Will fix.
…On Wed, Aug 19, 2020 at 7:15 AM David Taylor ***@***.***> wrote:
***@***.**** approved this pull request.
Looks like you picked up the go 1.14 proto diff
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#52836 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVDDSR4GQED3QSSU5C3SBOX55ANCNFSM4P72C6AA>
.
|
Add ability to record schedule groups: set of related schedules. Use this functionality to makean incremental schedule wait until the full one completes before it begins its execution. Release Notes: None
miretskiy
force-pushed
the
dep_schedule
branch
from
b8125ec to
f04cc3c
Compare
|
bors r+ |
|
Build succeeded: |
Fixes #52835
Add ability to record schedule groups: set of related schedules.
Use this functionality to makean incremental schedule wait
until the full one completes before it begins its execution.
Release Notes: None