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

jobs, backupccl: DROP SCHEDULE clears DependentID and releases PTS #69204

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

adityamaru
Copy link
Contributor

jobs, backupccl: DROP SCHEDULE clears DependentID and releases PTS
This change teaches DROP SCHEDULE to clear the dependent ID
linkage from the dependent schedule.

It also releases the protected timestamp (if any) on the schedule
being dropped. This pts would have been written if chaining of pts
was enabled.

Release note: None

backupccl: fix SHOW CREATE SCHEDULE options
In the previous commit DROP SCHEDULE wipes the dependent
ID linkage. SHOW CREATE SCHEDULE on an incremental schedule
whose dependent full schedule has been dropped will now
print a CREATE SCHEDULE statement with the full backup
recurrence cleared. This means that if the user were to copy
paste the command, we would choose a full backup recurrence
based on the incremental recurrence.

This commit also fixes the schedule options that are printed
as the output of SHOW CREATE SCHEDULE. Previously, these
would be printed as the string representations of the enum
values, but this was not round-trippable.

Informs: #69129

Release note: None

@adityamaru adityamaru requested review from dt, miretskiy and a team August 20, 2021 21:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru removed request for a team August 20, 2021 21:42
Copy link
Contributor

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, and @miretskiy)


pkg/ccl/backupccl/schedule_exec.go, line 393 at r1 (raw file):

}

// OnDrop implements the ScheduledJobController interface.

perhaps quick description of what it does (drops dependent schedule, etc)...


pkg/jobs/scheduled_job_executor.go, line 68 at r1 (raw file):

	// SCHEDULE` query.
	OnDrop(ctx context.Context, ie sqlutil.InternalExecutor, ptsProvider protectedts.Provider,
		env scheduledjobs.JobSchedulerEnv, schedule *ScheduledJob, txn *kv.Txn) error

It would be nice if we didn't expose ptsProvider as a top level argument.
Either we can pass full sql.ExecutorConfig to this function, or, and I think that would be my preference,
introduce

type ScheduleControllerExecConfig struct {
   ....
}

Or even

type ScheduleControllerEnv interface {
   InternalExecutor()
   PTSProvider()
}

(and just wrap executor config with a struct implementing above small interface).

@miretskiy miretskiy self-requested a review August 21, 2021 15:21
@adityamaru adityamaru force-pushed the drop-schedule-improvements branch 2 times, most recently from 9a0583a to 9dac899 Compare August 22, 2021 17:21
This change teaches `DROP SCHEDULE` to clear the dependent ID
linkage from the dependent schedule.

It also releases the protected timestamp (if any) on the schedule
being dropped. This pts would have been written if chaining of pts
was enabled.

Release note: None
In the previous commit `DROP SCHEDULE` wipes the dependent
ID linkage. `SHOW CREATE SCHEDULE` on an incremental schedule
whose dependent full schedule has been dropped will now
print a `CREATE SCHEDULE` statement with the full backup
recurrence cleared. This means that if the user were to copy
paste the command, we would choose a full backup recurrence
based on the incremental recurrence.

This commit also fixes the schedule options that are printed
as the output of `SHOW CREATE SCHEDULE`. Previously, these
would be printed as the string representations of the enum
values, but this was not round-trippable.

Informs: cockroachdb#69129

Release note: None
@adityamaru adityamaru force-pushed the drop-schedule-improvements branch from 9dac899 to 2e809e4 Compare August 22, 2021 21:36
@adityamaru
Copy link
Contributor Author

Unrelated bazel time out.

TFTR!

bors r=miretskiy

Copy link
Contributor Author

@adityamaru adityamaru left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt and @miretskiy)


pkg/ccl/backupccl/schedule_exec.go, line 393 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

perhaps quick description of what it does (drops dependent schedule, etc)...

Done.


pkg/jobs/scheduled_job_executor.go, line 68 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

It would be nice if we didn't expose ptsProvider as a top level argument.
Either we can pass full sql.ExecutorConfig to this function, or, and I think that would be my preference,
introduce

type ScheduleControllerExecConfig struct {
   ....
}

Or even

type ScheduleControllerEnv interface {
   InternalExecutor()
   PTSProvider()
}

(and just wrap executor config with a struct implementing above small interface).

Done. ExecutorConfig is in SQL pkg which causes an import cycle in scheduledjobs, so I just stored what we need in the lightweight struct.

@craig
Copy link
Contributor

craig bot commented Aug 23, 2021

Build succeeded:

@craig craig bot merged commit b711b24 into cockroachdb:master Aug 23, 2021
@adityamaru adityamaru deleted the drop-schedule-improvements branch August 23, 2021 13:35
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