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

roachtest: fix backup2TB by using IMPORT, rip out store dumps #36832

Merged
merged 3 commits into from
Apr 15, 2019

Conversation

tbg
Copy link
Member

@tbg tbg commented Apr 15, 2019

Now that 19.1 is tagged, the store dumps don't work any more. Consequently
the tests haven't run in over two weeks. Before that, it had episodes
during which it didn't run most of the time due to flakes downloading the
store dumps. Hopefully now it'll be more reliable.

Rip out the unused remainders of store dumps as well.

Fixes #34702.

Release note: None

@tbg tbg requested a review from bobvawter April 15, 2019 13:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from petermattis April 15, 2019 13:03
Copy link
Member

@bobvawter bobvawter left a comment

Choose a reason for hiding this comment

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

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


pkg/cmd/roachtest/backup.go, line 39 at r3 (raw file):

			// bank.bank doesn't exist, probably due to some gossip propagation
			// delay.
			delay := 5 * time.Second

Can you change this to poll for the existence of bank.bank before launching the BACKUP command? Then a longer timeout, maybe 1 minute, could be used before bailing out.


pkg/cmd/roachtest/cluster.go, line 530 at r3 (raw file):

}

func (s *clusterSpec) String() string {

Is this change intentional?

Copy link
Member Author

@tbg tbg 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 @bobvawter and @petermattis)


pkg/cmd/roachtest/backup.go, line 39 at r3 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Can you change this to poll for the existence of bank.bank before launching the BACKUP command? Then a longer timeout, maybe 1 minute, could be used before bailing out.

BTW, filed this issue as #36841. This is a bug.

I'll make that change, though I think it's largely an idle diversion. If it takes more than 5s, I'm happy to fail the test. And the test takes many hours, so the 5s don't matter.


pkg/cmd/roachtest/cluster.go, line 530 at r3 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

Is this change intentional?

Yes, with the pointer receiver you couldn't call this when you had a value. Now that it has a value receiver it works both on a clusterSpec and a *clusterSpec. For example, the new code in registerBackup makes use of this.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


pkg/cmd/roachtest/backup.go, line 39 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

BTW, filed this issue as #36841. This is a bug.

I'll make that change, though I think it's largely an idle diversion. If it takes more than 5s, I'm happy to fail the test. And the test takes many hours, so the 5s don't matter.

Nit: const delay = 5 * time.Second.

tbg added 3 commits April 15, 2019 19:22
Now that 19.1 is tagged, the store dumps don't work any more.
Consequently the tests haven't run in over two weeks. Before
that, it had episodes during which it didn't run most of the
time due to flakes downloading the store dumps. Hopefully
now it'll be more reliable.

Fixes cockroachdb#34702.

Release note: None
We're unlikely to want to bring them back.

Release note: None
It doesn't seem like we're planning on using store dumps in
the future. Our efforts should be going into improving IMPORT
instead.

Release note: None
Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r=petermattis

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bobvawter and @petermattis)


pkg/cmd/roachtest/backup.go, line 39 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: const delay = 5 * time.Second.

I did it The Bob Way.

craig bot pushed a commit that referenced this pull request Apr 15, 2019
36816: sql: move gossipccl.DisableMerges to gossip, call during idx backfill r=dt a=dt

This moves the DisableMerges helper from gossipccl to gossip, and adds a call to it during index backfills.

Release note: none.

36832: roachtest: fix backup2TB by using IMPORT, rip out store dumps r=petermattis a=tbg

Now that 19.1 is tagged, the store dumps don't work any more. Consequently
the tests haven't run in over two weeks. Before that, it had episodes
during which it didn't run most of the time due to flakes downloading the
store dumps. Hopefully now it'll be more reliable.

Rip out the unused remainders of store dumps as well.

Fixes #34702.

Release note: None

Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Copy link
Member

@bobvawter bobvawter 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 4 files at r4, 2 of 2 files at r5, 2 of 2 files at r6.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@craig
Copy link
Contributor

craig bot commented Apr 15, 2019

Build succeeded

@craig craig bot merged commit 5cfca6b into cockroachdb:master Apr 15, 2019
pbardea added a commit to pbardea/cockroach that referenced this pull request Sep 20, 2019
This test has been flaky because the TableDescriptor being read is being
seen as OFFLINE. This issue seems to disappear after waiting a bit. This
also happens during IMPORT and this issue was avoided in a similar way
(see cockroachdb#36832).

This should be removed and cockroachdb#40951 is tracking this issue.

Release justification: Work-around for a flaky test.

Release note: None
pbardea added a commit to pbardea/cockroach that referenced this pull request Sep 20, 2019
This test has been flaky because the TableDescriptor being read is being
seen as OFFLINE. This issue seems to disappear after waiting a bit. This
also happens during IMPORT and this issue was avoided in a similar way
(see cockroachdb#36832).

This should be removed and cockroachdb#40951 is tracking this issue.

Release justification: Work-around for a flaky test.

Release note: None
pbardea added a commit to pbardea/cockroach that referenced this pull request Sep 23, 2019
This test has been flaky because the TableDescriptor being read is being
seen as OFFLINE. This issue seems to disappear after waiting a bit. This
also happens during IMPORT and this issue was avoided in a similar way
(see cockroachdb#36832).

This should be removed and cockroachdb#40951 is tracking this issue.

Release justification: Work-around for a flaky test.

Release note: None
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.

roachtest: backup2TB failed
4 participants