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

importccl: Rollback partial IMPORT INTO #39459

Merged
merged 4 commits into from
Aug 9, 2019
Merged

Conversation

dt
Copy link
Member

@dt dt commented Aug 8, 2019

importccl: Rollback partial IMPORT INTO

This uses RevertRange to rollback IMPORT’ed data when IMPORT fails.
Without this, partially imported data would remain in the table — which
could lead to unexpected results, prevent retrying (due to uniqueness)
and generally make things messy.

Release note (sql change): IMPORT INTO cleans up any imported rows if it fails.

jobs,importccl: add testing knobs to job resumers

Jobs are run by resumers, which are created by the registry.
Any testing knobs in a job's execution therefore need to be plumbed via
the resumer, which in turn needs to be plumbed from the registry.

This adds a generic hook that can be installed to run during resumer creation
and an example of it for IMPORT for failure injection after a job is otherwise
complete (e.g. so it has the most to clean up).

Release note: none.

importccl: fix no-op txn against unused server

Somehow ended up with a testserver as well as the testcluster.
This code meant to insert X rows in one txn, but the txn was against the
single server while the inserts were being sent (standalone) to the cluster.

This simply removes the extra server and the unused txns.

Release note: none.

sql: log when reverting tables

Reverting a table is a big deal, and could easily trip up other systems
so if someone is looking at logs around the time a revert happened, it
would probably be useful for it to be mentioned there.

Release note: none.

@dt dt requested a review from adityamaru27 August 8, 2019 16:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@adityamaru27 adityamaru27 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt)


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

ifm

Nit: extra m


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

	// admin knob (e.g. ALTER TABLE REVERT TO SYSTEM TIME) if anything goes wrong.

That's a good idea. Out of curiosity would we be able to execute that time travel over here as the last resort if reverting fails? Or since it will potentially use the same code path, we're confident that it will fail since our revert failed?


pkg/ccl/importccl/import_stmt_test.go, line 1230 at r2 (raw file):

	defer tc.Stopper().Stop(ctx)
	conn := tc.Conns[0]
	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})

Oops this was definitely me, I think I just copy-pasted the logic without realizing that was what was going on. Thanks for the cleanup.

Copy link
Member Author

@dt dt 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 @adityamaru27)


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

Previously, adityamaru27 (Aditya Maru) wrote…
ifm

Nit: extra m

Done.


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

Previously, adityamaru27 (Aditya Maru) wrote…
	// admin knob (e.g. ALTER TABLE REVERT TO SYSTEM TIME) if anything goes wrong.

That's a good idea. Out of curiosity would we be able to execute that time travel over here as the last resort if reverting fails? Or since it will potentially use the same code path, we're confident that it will fail since our revert failed?

I believe time travel works, since we'll resolve the pre-offline desc at the time of the query, and then not see the state.


pkg/ccl/importccl/import_stmt_test.go, line 1230 at r2 (raw file):

Previously, adityamaru27 (Aditya Maru) wrote…

Oops this was definitely me, I think I just copy-pasted the logic without realizing that was what was going on. Thanks for the cleanup.

👍

Reverting a table is a big deal, and could easily trip up other systems
so if someone is looking at logs around the time a revert happened, it 
would probably be useful for it to be mentioned there.

Release note: none.
@dt dt force-pushed the import-rollback branch from ff7d2e1 to 0cfcc2c Compare August 8, 2019 21:14
dt added 3 commits August 9, 2019 15:54
Somehow ended up with a testserver as well as the testcluster.
This code meant to insert X rows in one txn, but the txn was against the
single server while the inserts were being sent (standalone) to the cluster.

This simply removes the extra server and the unused txns.

Release note: none.
Jobs are run by resumers, which are created by the registry.
Any testing knobs in a job's execution therefore need to be plumbed via
the resumer, which in turn needs to be plumbed from the registry.

This adds a generic hook that can be installed to run during resumer
creation and an example of it for IMPORT for failure injection after a
job is otherwise complete (e.g. so it has the most to clean up).

Release note: none.
This uses RevertRange to rollback IMPORT’ed data when IMPORT fails.
Without this, partially imported data would remain in the table — which
could lead to unexpected results, prevent retrying (due to uniqueness)
and generally make things messy.

Release note (sql change): IMPORT INTO cleans up any imported rows if it fails.
@dt dt force-pushed the import-rollback branch from 0cfcc2c to 3621580 Compare August 9, 2019 15:55
@dt
Copy link
Member Author

dt commented Aug 9, 2019

bors r+

craig bot pushed a commit that referenced this pull request Aug 9, 2019
39459:  importccl: Rollback partial IMPORT INTO r=dt a=dt

importccl: Rollback partial IMPORT INTO

This uses RevertRange to rollback IMPORT’ed data when IMPORT fails.
Without this, partially imported data would remain in the table — which
could lead to unexpected results, prevent retrying (due to uniqueness)
and generally make things messy.

Release note (sql change): IMPORT INTO cleans up any imported rows if it fails.

jobs,importccl: add testing knobs to job resumers

Jobs are run by resumers, which are created by the registry.
Any testing knobs in a job's execution therefore need to be plumbed via
the resumer, which in turn needs to be plumbed from the registry.

This adds a generic hook that can be installed to run during resumer creation
and an example of it for IMPORT for failure injection after a job is otherwise
complete (e.g. so it has the most to clean up).

Release note: none.

importccl: fix no-op txn against unused server

Somehow ended up with a testserver as well as the testcluster.
This code meant to insert X rows in one txn, but the txn was against the
single server while the inserts were being sent (standalone) to the cluster.

This simply removes the extra server and the unused txns.

Release note: none.

sql: log when reverting tables

Reverting a table is a big deal, and could easily trip up other systems
so if someone is looking at logs around the time a revert happened, it
would probably be useful for it to be mentioned there.

Release note: none.

Co-authored-by: David Taylor <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 9, 2019

Build succeeded

@craig craig bot merged commit 3621580 into cockroachdb:master Aug 9, 2019
@dt dt deleted the import-rollback branch August 9, 2019 21:47
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