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

gc: end to end integration test #87516

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

aliher1911
Copy link
Contributor

@aliher1911 aliher1911 commented Sep 7, 2022

Currently tests are covering various components, but there's no end to end flow test that would verify that GC will issue GC requests that would successfully remove all relevant data.
This commit adds a TestCluster based test to verify GC behaviour.

Release justification: test only change
Release note: None
Fixes #84988

@aliher1911 aliher1911 requested a review from a team September 7, 2022 17:49
@aliher1911 aliher1911 requested a review from a team as a code owner September 7, 2022 17:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@aliher1911 aliher1911 self-assigned this Sep 7, 2022
@aliher1911 aliher1911 force-pushed the gc_on_test_cluster branch 3 times, most recently from 9a98d74 to c182ccf Compare September 8, 2022 09:28
@tbg
Copy link
Member

tbg commented Sep 8, 2022

Feel free to request direct review when ready, I think you're probably still fiddling with this given our recent slack convo ;-)

@aliher1911
Copy link
Contributor Author

Those were just minor tweaking of TestCluster settings and it looks like it passed all the stress and race. Should be ok to review.

@aliher1911 aliher1911 requested a review from tbg September 8, 2022 12:03
Copy link
Member

@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.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911)


pkg/kv/kvserver/gc/gc_int_test.go line 73 at r1 (raw file):

	statusServer := tc.Server(0).StatusServer().(serverpb.StatusServer)

	execOrFatal := func(stmt string, args ...interface{}) {

Take t *testing.T here, and call t.Helper(), that way you can tell which caller fataled.


pkg/kv/kvserver/gc/gc_int_test.go line 78 at r1 (raw file):

	}

	getTableRangeIDs := func(db *gosql.DB) ids {

Please pass *testing.T in all the methods and call t.Helper.


pkg/kv/kvserver/gc/gc_int_test.go line 91 at r1 (raw file):

	}

	readSomeKeys := func(db *gosql.DB) []int64 {

ditto


pkg/kv/kvserver/gc/gc_int_test.go line 103 at r1 (raw file):

	}

	getRangeInfo := func(rangeID int64, db *gosql.DB) (startKey, endKey []byte) {

ditto


pkg/kv/kvserver/gc/gc_int_test.go line 111 at r1 (raw file):

	}

	deleteRangeDataWithRangeTombstone := func(rangeIDs []int64, kvDb *kv.DB) {

ditto


pkg/kv/kvserver/gc/gc_int_test.go line 119 at r1 (raw file):

	}

	getRangeStats := func(rangeID int64) enginepb.MVCCStats {

ditto


pkg/kv/kvserver/gc/gc_int_test.go line 129 at r1 (raw file):

	}

	findNonEmptyRanges := func(rangeIDs ids) (nonEmptyRangeIDs ids) {

ditto


pkg/kv/kvserver/gc/gc_int_test.go line 132 at r1 (raw file):

		for _, id := range rangeIDs {
			stats := getRangeStats(id)
			t.Logf("range %d stats: %s", id, &stats)

Need to check somewhere that ContainsEstimates is false.


pkg/kv/kvserver/gc/gc_int_test.go line 165 at r1 (raw file):

	require.Empty(t, readSomeKeys(sqlDb), "table still contains data after range deletion")

	execOrFatal(`alter table kv split at values ($1)`, math.MaxInt64/2)

Why? And why now?

Copy link
Contributor Author

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


pkg/kv/kvserver/gc/gc_int_test.go line 132 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Need to check somewhere that ContainsEstimates is false.

Can we have ContainsEstimates true here? I thought they are produced by imports and the such operations. If there are estimates, we won't be able to assert the test result and we can't reasonably recover. We can fail the whole test though.


pkg/kv/kvserver/gc/gc_int_test.go line 165 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why? And why now?

Checking that range that was split by range split is still deleted correctly.

Copy link
Member

@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 @aliher1911)


pkg/kv/kvserver/gc/gc_int_test.go line 132 at r1 (raw file):

Previously, aliher1911 (Oleg) wrote…

Can we have ContainsEstimates true here? I thought they are produced by imports and the such operations. If there are estimates, we won't be able to assert the test result and we can't reasonably recover. We can fail the whole test though.

Right, just assert it's false basically.


pkg/kv/kvserver/gc/gc_int_test.go line 165 at r1 (raw file):

Previously, aliher1911 (Oleg) wrote…

Checking that range that was split by range split is still deleted correctly.

Maybe add a comment, seems kind of unclear why, but it doesn't hurt either. Is there any interaction with splits that is interesting here? You push them through the queue separately, so it looks the same to me, just doing it twice rather than once.

Copy link
Member

@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 @aliher1911)

Currently tests are covering various components, but there's no end
to end flow test that would verify that GC will issue GC requests that
would successfully remove all relevant data.
This commit adds a TestCluster based test to verify GC behaviour.

Release justification: test only change
Release note: None
Copy link
Contributor Author

@aliher1911 aliher1911 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 @tbg)


pkg/kv/kvserver/gc/gc_int_test.go line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Please pass *testing.T in all the methods and call t.Helper.

What is the benefit of passing t to every helper lambda here, to avoid accessing variables from outer scope? Currently it's a mixed bag, some functions receive connection and some use it from test function itself, I'll fix that as well then.


pkg/kv/kvserver/gc/gc_int_test.go line 165 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Maybe add a comment, seems kind of unclear why, but it doesn't hurt either. Is there any interaction with splits that is interesting here? You push them through the queue separately, so it looks the same to me, just doing it twice rather than once.

You are right, I now think it doesn't belong here. Updating stats should be tested on split/merge separately.

@aliher1911
Copy link
Contributor Author

bors r=tbg

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build succeeded:

@craig craig bot merged commit 5b8a6e2 into cockroachdb:master Sep 8, 2022
Copy link
Member

@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


pkg/kv/kvserver/gc/gc_int_test.go line 78 at r1 (raw file):

Previously, aliher1911 (Oleg) wrote…

What is the benefit of passing t to every helper lambda here, to avoid accessing variables from outer scope? Currently it's a mixed bag, some functions receive connection and some use it from test function itself, I'll fix that as well then.

https://play.golang.com/p/hWebTlxd9Nk

It makes sure that the errors that get created are made to originate from the line of the caller, not the line of the helper. Note how the calls to badHelper can't be traced back to where they were called from.
This is less of an issue with using require because that always logs a stack trace anyway, but it's better to pass t explicitly just as muscle memory, since not everything uses require.

@aliher1911
Copy link
Contributor Author

I don't need to pass t to make helper work as I think it .Helper() only ensures stack frames where helper was invoked are ignored:
https://play.golang.com/p/LREvYRQC5aQ

So we can achieve the same with just using t from main test function. I'm more interested in, if I pass t, then I'd rather pass everything and make this a proper function. I don't see why t has any special meaning here. Either go full on with inheriting everything or pass everything every time. Imho for function literals there's little benefit of passing all shared variables as you will always pass the same values within context of your test anyways.

@tbg
Copy link
Member

tbg commented Sep 9, 2022

Yeah, fair point.

@erikgrinaker
Copy link
Contributor

This only seems to test the range tombstone full-range fast path? Shouldn't we also, at minimum, test that point keys and range keys are GCed correctly, both for updates and deletes?

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.

end-to-end test cases for GC are missing
4 participants