storage: extend code coverage for snapshot tests#45100
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Feb 27, 2020
Merged
storage: extend code coverage for snapshot tests#45100craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
04d3168 to
086ecf3
Compare
Contributor
|
As discussed in person, @OwenQian is going to ping this issue once the tests are stable under stress. |
cf8f9af to
50c5ba9
Compare
nvb
approved these changes
Feb 27, 2020
Contributor
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @OwenQian)
pkg/storage/client_merge_test.go, line 2895 at r1 (raw file):
} // Only try to predict SSTS 3 and 5-7. SSTs 1, 2 and 4 are excluded in
nit: s/SSTS/SSTs/
pkg/storage/client_merge_test.go, line 2911 at r1 (raw file):
// keep the 3rd one. keyRanges := rditer.MakeReplicatedKeyRanges(inSnap.State.Desc) it := rditer.NewReplicaDataIterator(inSnap.State.Desc, sendingEng, true, false)
For boolean arguments, our style guide advises that we include a comment with the param name: true /* replicatedOnly */, false /* seekEnd */.
pkg/storage/client_merge_test.go, line 2992 at r1 (raw file):
} if !bytes.Equal(actualSST, expectedSSTs[i]) { mismatchedSstsIdx = append(mismatchedSstsIdx, i)
We can get rid of this debug code now that we're passing. It's nice that we have this, but it's a little too much code to maintain for a failure path.
pkg/storage/replica_sst_snapshot_storage_test.go, line 110 at r1 (raw file):
defer leaktest.AfterTest(t)() ctx := context.TODO()
nit: context.Background() instead. This was in flux for a while, but we eventually settled on Background() - #14220.
This PR extends the testing coverage by adding a unit test for MultiSSTWriter and extends TestStoreRangeMergeRaftSnapshot by predicting the contents of the SST containing user keys. This change is made to extend the testing coverage in preparation for a behavior change in what's written to the SST files when receiving a snapshot. NB: Originally, the plan was to extend TestStoreRangeMergeRaftSnapshot by adding test coverage of the following SSTs: 1. Replicated range-id local keys of the range in the snapshot. 2. Range-local keys of the range in the snapshot. 3. User keys of the range in the snapshot. but this PR only adds coverage for the user keys (SST number 3), since it turns out that trying to predict the first two SSTs makes the test flaky because there seems to be some non-determinism with the snapshot sender's Raft log after sending the snapshot. Every so often, the RaftAppliedIdx of the expected and actual SSTs will be mismatched. Release note: None. Deflake TestStoreRangeMergeRaftSnapshot
50c5ba9 to
aa2d5a2
Compare
Author
|
bors r=nvanbenschoten |
Contributor
Build succeeded |
craig bot
pushed a commit
that referenced
this pull request
Mar 6, 2020
44725: storage: constrained span of rangedel in ClearRange to keys in range r=nvanbenschoten a=OwenQian Constrains the width of the range deletion tombstone to the span of keys actually present within the range. If the range has no kv-entries, then skip the rangedel completely. Before this change, when receiving a snapshot, the original file would have a range deletion tombstone that spanned the entire range written to it regardless of the actual keys contained in the range or if the range was empty. This resulted in the creation of excessively wide tombstones, which has significant performance implications since the wide tombstones impede compaction. Fixes #44048. Rebased off #45100. Release note: None. 45157: sql: add inverted indexes on arrays r=jordanlewis a=jordanlewis Closes #43199. This commit adds inverted index support to arrays. Inverted index entries are created from arrays by simply encoding a key that contains the array element's table key encoding. Nulls are not indexed, since in SQL, ARRAY[1, NULL] @> ARRAY[NULL] returns false. For example, in a table t(int, int[]) with an inverted index with id 3 on the int[] column the row (10, [1, NULL, 2]) produces 2 index keys: ``` /tableId/3/1/10 /tableId/3/2/10 ``` This encoding scheme is much simpler than the one for JSON, since arrays don't have "paths": their elements are simply ordinary datums. Release note (sql change): The inverted index implementation now supports indexing array columns. This permits accelerating containment queries (@> and <@) on array columns by adding an index to them. 45642: ui: Set react component `key` prop to fix react errors r=nathanstilwell a=koorosh Set react component `key` prop to fix react errors Resolves: #45188 Co-authored-by: Owen Qian <owenq@cockroachlabs.com> Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR extends the testing coverage by adding a unit test for MultiSSTWriter and extends TestStoreRangeMergeRaftSnapshot by predicting the contents of the SST containing user keys.
This change is made to extend the testing coverage in preparation for a behavior change in what's written to the SST files when receiving a snapshot.
NB: Originally, the plan was to extend TestStoreRangeMergeRaftSnapshot by adding test coverage of the following SSTs:
but this PR only adds coverage for the user keys (SST number 3), since it turns out that trying to predict the first two SSTs makes the test flaky because there seems to be some non-determinism with the snapshot sender's Raft log after sending the snapshot. Every so often, the RaftAppliedIdx of the expected and actual SSTs will be mismatched.
Release note: None.