jobs,importccl: adopt protected timestamps for IMPORT #44739

ajwerner · 2020-02-05T01:43:50Z

This PR comes in two commits. See each for details.

The first refactors and augments the jobs.Registry API to enable transactional creation of a job which will be started on the local gateway. I would love to discuss this commit itself a bit.
Adopt protected timestamps and demonstrate their effectiveness with a test.

cockroach-teamcity · 2020-02-05T01:43:58Z

This change is

ajwerner · 2020-02-05T13:22:02Z

cc @lucy-zhang this is the PR with the jobs change

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/jobs/registry.go, line 286 at r3 (raw file):

// StartableJob is a job created with a transaction to be started later.
// See Registry.CreateStartableJob
type StartableJob struct {

please move to jobs.go

pkg/jobs/registry.go, line 304 at r3 (raw file):

		}
	}()
	// TODO(ajwerner): Consider adding some protection against starting more than

can you add detail on how that can happen?

pkg/jobs/registry.go, line 320 at r3 (raw file):

// CreateStartableJobWithTxn creates a job to be started later, after the
// creating txn commits. It stores writes job in the jobs table, marks it

... stores writes ... ?

pkg/jobs/registry.go, line 332 at r3 (raw file):

		return nil, err
	}
	j.WithTxn(nil)

please add a comment why we need this

pkg/jobs/registry.go, line 338 at r3 (raw file):

	}
	resumerCtx, cancel := r.makeCtx()
	if err := r.register(*j.ID(), cancel); err != nil {

if the txn is aborted the job will not be unregistered.

spaskob

Reviewed 5 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/ccl/importccl/import_stmt.go, line 630 at r4 (raw file):

StartableJob

why can't we do that here using using QueueJob from planner.go?

ajwerner · 2020-02-07T00:06:36Z

cc @lucy-zhang this has the jobs change we talked about - @spaskob and I discussed offline that you should certainly merge your change that utilized QueueJob and Run as written today and then later that can be refactored. This doesn't touch that API at all, just an FYI and some reassurance that any API changes in jobs will be out of your way.

dt · 2020-02-19T22:31:50Z

pkg/ccl/importccl/import_stmt.go

+		// Prepare the protected timestamp record.
+		spans := make([]roachpb.Span, 0, len(tableDetails))
+		for i := range tableDetails {
+			spans = append(spans, tableDetails[i].Desc.TableSpan())


I think we only need to add spans for tables where IsNew is false (which could require revert) and then below we can skip if spans is empty

ajwerner

This is RFAL.

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

pkg/ccl/importccl/import_stmt.go, line 630 at r4 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

StartableJob
why can't we do that here using using QueueJob from planner.go?

I think we discussed this offline.

pkg/ccl/importccl/import_stmt.go, line 652 at r4 (raw file):

Previously, dt (David Taylor) wrote…

I think we only need to add spans for tables where IsNew is false (which could require revert) and then below we can skip if spans is empty

Done.

pkg/jobs/registry.go, line 286 at r3 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

please move to jobs.go

Done.

pkg/jobs/registry.go, line 304 at r3 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

can you add detail on how that can happen?

If there's a programming error in the client. I added protection.

pkg/jobs/registry.go, line 320 at r3 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

... stores writes ... ?

Fixed.

pkg/jobs/registry.go, line 332 at r3 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

please add a comment why we need this

Done.

pkg/jobs/registry.go, line 338 at r3 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

if the txn is aborted the job will not be unregistered.

Good point! I've extended the interface to deal with that case and added some testing. The contract is now that user transactions which are rolled back will need to be cleaned up using CleanupOnRollback. I also added unit testing.

ajwerner · 2020-02-26T22:39:07Z

erm actually let me fix the test, must have broken something

ajwerner

Okay, RFAL now

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

ajwerner · 2020-02-27T15:08:55Z

@spaskob can you give the first commit this a look? I'd like to get this merged ASAP

spaskob · 2020-02-27T15:49:40Z

ok looking now

…

On Thu, Feb 27, 2020 at 10:12 AM ajwerner ***@***.***> wrote: @spaskob <https://github.com/spaskob> can you give the first commit this a look? I'd like to get this merged ASAP — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44739?email_source=notifications&email_token=AANL635UYHO7YVKRVLFS2PLRE7J53A5CNFSM4KQCXN6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENEWQZY#issuecomment-592013415>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANL633QVKEM627VOFKKCG3RE7J53ANCNFSM4KQCXN6A> .

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/jobs/jobs.go, line 740 at r5 (raw file):

}

// Start will resume a resumable job. The transaction used to create the

Start will start a job; resume can be confused with RESUME job_id

pkg/jobs/jobs_test.go, line 1994 at r5 (raw file):

}

// TestStartableJobErrors tests that the StartableJob returns the expected

I know you want to get this merged but a good to have is to search for use of CreateAndStartJob in this test file sand see if it may be useful to add similar test or replace with a StartableJob.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/jobs/jobs_test.go, line 1994 at r5 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

I know you want to get this merged but a good to have is to search for use of CreateAndStartJob in this test file sand see if it may be useful to add similar test or replace with a StartableJob.

I don't know that I follow. CreateAndStartJob is implemented on top of StartableJob. All tests which exercise it exercise the StartableJob code.

spaskob

ok great then, I missed that.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

ajwerner · 2020-02-29T00:06:11Z

The remaining test failures are addressed by #45544

spaskob

LGTM for jobs code

Reviewed 1 of 7 files at r5, 4 of 9 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/ccl/importccl/import_processor_test.go, line 611 at r6 (raw file):

pkg/ccl/importccl/import_processor_test.go, line 612 at r6 (raw file):

			resumer := raw.(*importResumer)

StartableJob isn't great but it's better than what we've got now. A StartableJob is a job which is created transactionally but also hooked in to a `resultCh` and can be started on the local gateway after the transaction commits. Currently we've got two main ways that jobs are kicked off "synchronously": 1) Registry.Run() This doesn't currently do anything to kick off the jobs - it just waits for all of the relevant jobs to be started. This is used in conjunction with `planner.QueueJob` and `CreateJobWithTxn`. The idea here is that when a transaction creates a job we want, upon commit, for it to wait for the jobs it created to complete. 2) Registry.CreateAndStartJob() This method has special powers that prevents the gateway node from adopting the job and also writes the job with a lease for the local node. It also allows the caller to pass a resultsCh which will recieve datums from the job. That resultCh is critical for IMPORT and BACKUP (?). Currently in both of those cases we create and run the job on a transaction outside of the caller's txn. This is particularly frustrating because the jobs package assigns IDs at creation time, making it difficult to transactionally associate a job with another resource while also retaining the ability to ensure that it starts locally and is connected to a resultsCh. This commit addresses that. The fact that `IMPORT` and `BACKUP` commit the job before committing the planner's txn feels dirty. I'm not planning on mucking with that stuff here in this PR but ultimately the work here seems to lay a possible path forward. In particular, instead of calling `Run()` with a slice of IDs we could call it with a slice of `StartableJob`s which may be hooked up to the channel. In that way we could unify CreateAndStartJob() and Run() with something better. Release note: None.

Perhaps this only needed to be applied to IMPORT INTO but it doesn't hurt import. This change ensures that IMPORT statements protect the spans into which they'll be writing from being GC'd. The change adopts the StartableJob change from the previous commit, protects a timestamp, and then makes sure to clean up that timestamp on success or failure. Before this change the test added here would fail with the following error: ``` require.go:1104: Error Trace: import_into_test.go:152 Error: Expect "pq: job 526896149330362369: cannot be reverted, manual cleanup may be required: rolling back partially completed IMPORT: cannot revert before replica GC threshold 1580866345.938283735,0" to match "error response from server: 500 Internal Server Error" Test: TestProtectedTimestampsDuringImportInto ``` Release note (bug fix): IMPORT INTO jobs which are canceled or fail can no longer get stuck in an unrecoverable state if data from the previous state of the table had expired relative to the GC TTL.

The logic to detect whether a type is clonable cannot traverse into all of the implementations of itnerfaces. This is problematic when one of the structs underneath a oneof is unclonable. While a more general solution might be nice, this workaround allows the addition of a UUID field to a job details struct. Release note: None

ajwerner

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @spaskob)

pkg/jobs/jobs.go, line 740 at r5 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

Start will start a job; resume can be confused with RESUME job_id

Done.

craig · 2020-02-29T19:07:25Z

Build succeeded

GitHub CI (Cockroach)

ajwerner requested review from dt and spaskob February 5, 2020 01:43

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch 2 times, most recently from 3a084b5 to bb36776 Compare February 5, 2020 05:36

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch from bb36776 to d965916 Compare February 5, 2020 13:50

spaskob suggested changes Feb 6, 2020

View reviewed changes

spaskob mentioned this pull request Feb 6, 2020

jobs, *: refactor planning vs exec APIs #37691

Closed

dt approved these changes Feb 19, 2020

View reviewed changes

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch from d965916 to d8b6f38 Compare February 26, 2020 20:31

ajwerner commented Feb 26, 2020

View reviewed changes

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch from d8b6f38 to 9adacc9 Compare February 26, 2020 22:57

ajwerner commented Feb 26, 2020

View reviewed changes

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch 3 times, most recently from dc340d3 to 607ea4a Compare February 27, 2020 05:46

spaskob approved these changes Feb 27, 2020

View reviewed changes

ajwerner commented Feb 27, 2020

View reviewed changes

spaskob approved these changes Feb 27, 2020

View reviewed changes

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch 4 times, most recently from 4f92a1d to 8d32c56 Compare February 28, 2020 20:39

spaskob approved these changes Feb 29, 2020

View reviewed changes

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch from 8d32c56 to dd9f637 Compare February 29, 2020 09:25

ajwerner added 3 commits February 29, 2020 13:14

ajwerner force-pushed the ajwerner/adopt-protected-timestamps-in-import-into branch from dd9f637 to bfa371b Compare February 29, 2020 18:14

ajwerner commented Feb 29, 2020

View reviewed changes

craig bot merged commit 0dc604b into cockroachdb:master Feb 29, 2020

This was referenced Mar 8, 2020

backupccl: adopt protected timestamps for BACKUP #45859

Merged

server: job status endpoint crashes node #46049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jobs,importccl: adopt protected timestamps for IMPORT #44739

jobs,importccl: adopt protected timestamps for IMPORT #44739

ajwerner commented Feb 5, 2020

cockroach-teamcity commented Feb 5, 2020

ajwerner commented Feb 5, 2020

spaskob left a comment

spaskob left a comment

ajwerner commented Feb 7, 2020

dt Feb 19, 2020

ajwerner left a comment

ajwerner commented Feb 26, 2020

ajwerner left a comment

ajwerner commented Feb 27, 2020

spaskob commented Feb 27, 2020 via email

spaskob left a comment

ajwerner left a comment

spaskob left a comment

ajwerner commented Feb 29, 2020

spaskob left a comment

ajwerner left a comment

craig bot commented Feb 29, 2020

jobs,importccl: adopt protected timestamps for IMPORT #44739

jobs,importccl: adopt protected timestamps for IMPORT #44739

Conversation

ajwerner commented Feb 5, 2020

cockroach-teamcity commented Feb 5, 2020

ajwerner commented Feb 5, 2020

spaskob left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 7, 2020

dt Feb 19, 2020

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 26, 2020

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 27, 2020

spaskob commented Feb 27, 2020 via email

spaskob left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 29, 2020

spaskob left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

craig bot commented Feb 29, 2020

Build succeeded