bulkio: Support running BACKUP under transaction.

[miretskiy]

Informs #47539

Allow execution of BACKUP statement under explicit transaction,
provided the "DETACHED" option is specified:

BACKUP TO <location_uri> ... WITH DETACHED

When backup runs in detached mode, we do not wait for the job
completion. Instead, we return the job id to the caller, and
the job executes asynchronously.

Release notes (enterprise change): BACKUP can now run in "detached" mode.
That is, we do not wait for the BACKUP job to finish. Instead, the
job ID is returned immediately, and the job itself runs in background.

[cockroach-teamcity]

This change is 

[pbardea]

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

---

pkg/ccl/backupccl/backup_test.go, line 3473 at r1 (raw file):

}

func waitForSuccessfulJob(t *testing.T, tc *testcluster.TestCluster, id int64) {

By the way, we have something like this in jobsutil.WaitForJob that we use in some of the backup tests as well as import and create stats test.

Could we make that method nudge the registry and re-use it?

---

pkg/ccl/backupccl/backup_test.go, line 3492 at r1 (raw file):

	db := sqlDB.DB.(*gosql.DB)

	// running backup under transaction requires DETACHED

nit: comment sentence formatting

---

pkg/ccl/backupccl/backup_test.go, line 3507 at r1 (raw file):

	// Backup again, under explicit transaction.
	tx, err = db.Begin()
	require.NoError(t, err)

Thanks for cleaning these up!

---

pkg/ccl/backupccl/backup_test.go, line 3511 at r1 (raw file):

	require.NoError(t, err)
	require.NoError(t, tx.Commit())
	waitForSuccessfulJob(t, tc, jobID)

Can we also add a test for rolling back the transaction instead of committing?

---

pkg/jobs/registry.go, line 383 at r1 (raw file):

// CreateAdoptableJobWithTxn creates a job which will be adopted for execution at a later time
// by some node in a cluster.

nit: s/in a cluster/in the cluster/

---

pkg/jobs/registry.go, line 388 at r1 (raw file):

) (*Job, error) {
	j := r.NewJob(record)
	if err := j.WithTxn(txn).insert(

I think it's worth adding a comment explicitly mentioning that we purposely create a lease with an invalid node ID so that the registry will adopt it later.

---

pkg/jobs/registry.go, line 1033 at r1 (raw file):

	}
	nodeStatusMap := map[roachpb.NodeID]*nodeStatus{
		// 0 is not a valid node ID, but we treat it as an always-dead node so that

should we update this comment since it's not clear what 0 is referring to anymore? Something like "We treat invalidNodeID as an ..."

[pbardea]

Also wanted to flag that it looks like there are a couple of proto generated files included here - probably due to the Go version upgrade.

Also, I don't think "improvement" is a release category. I think the closest category would be "enterprise change".

[miretskiy]

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

---

pkg/ccl/backupccl/backup_test.go, line 3473 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

By the way, we have something like this in jobsutil.WaitForJob that we use in some of the backup tests as well as import and create stats test.

Could we make that method nudge the registry and re-use it?

I saw that... But, tbh, I wasn't comfortable adding nudge to that utility; and if I'm going to write a wrapper right here (to nudge and call jobsutil.WaitForJob), then I thought that it might actually be more readable if this happened inline, right here. If you feel strongly, I can switch.

---

pkg/ccl/backupccl/backup_test.go, line 3507 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Thanks for cleaning these up!

Done.

---

pkg/jobs/registry.go, line 388 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I think it's worth adding a comment explicitly mentioning that we purposely create a lease with an invalid node ID so that the registry will adopt it later.

Done.

---

pkg/jobs/registry.go, line 1033 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

should we update this comment since it's not clear what 0 is referring to anymore? Something like "We treat invalidNodeID as an ..."

Done.

[miretskiy]

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

---

pkg/ccl/backupccl/backup_test.go, line 3511 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Can we also add a test for rolling back the transaction instead of committing?

Done.

[miretskiy]

@pbardea @dt ready for another look;

[miretskiy]

bors r+

[miretskiy]

bors r-

[craig]

Canceled

[miretskiy]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[miretskiy]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[miretskiy]

bors r+

[craig]

Already running a review

[craig]

Build succeeded

• GitHub CI (Cockroach)

[dt]

closes #47539?

[miretskiy]

Closes #47539

[miretskiy]

yup

…

On Thu, Jul 2, 2020 at 8:06 PM David Taylor ***@***.***> wrote: closes #47539 <#47539>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50775 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVB6QPOVATAPFU6YFMLRZUOJHANCNFSM4OLQTRUQ> .