engine: redefine targetSize and add maxSize to ExportToSst#44553
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Jan 30, 2020
Merged
Conversation
Member
|
This change is |
added 2 commits
January 30, 2020 14:48
In cockroachdb#44440 we added a `targetSize` parameter to enable pagination of export requests. In that PR we defined the targetSize to return just before the key that would lead to the `targetSize` being exceeded. This definition is unfortunate when thinking about a total size limit for pagination in the DistSender (which we'll add when cockroachdb#44341 comes in). Imagine a case where we set a total byte limit of 1MB and a file byte limit of 1MB. That setting should lead to at most a single file being emitted (assuming one range holds enough data). If we used the previous definition we'd create a file which is just below 1MB and then the DistSender would need send another request which would contain a tiny amount of data. This brings the behavior in line with the semantics introduced in cockroachdb#44341 for ScanRequests and is just easier to reason about. Release note: None
It would be a major change to allow pagination of exported SSTs within versions of an individual key. Given that the previous changes always include all versions of a key, there's a concern that keys with very large numbers of versions could create SSTs which could not be restored or which might OOM a server. We'd rather fail to create a backup than OOM or create an unusable backup. To deal with this, this commit adds a new maxSize parameter above which the ExportToSst call will fail. In a follow-up commit there will be a cluster setting to configure this value, for now it is set to unlimited. If customers are to hit this error when creating a backup they'll need to either set a lower GC TTL and run GC or use a point-in-time backup rather than a backup which contains all of the versions. The export tests were extended to ensure that this parameter behaves as expected and was stressed on the teeing engine to ensure that the behavior matches between pebble and rocksdb. Relates to cockroachdb#43356 CC @dt Release note: None
e2b4a3e to
8429a42
Compare
itsbilal
approved these changes
Jan 30, 2020
Contributor
itsbilal
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 10 of 10 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/ccl/storageccl/export_test.go, line 359 at r2 (raw file):
require.Truef(t, targetSize > dataSizeWithoutLastKey, "%d <= %d", targetSize, dataSizeWithoutLastKey) // Ensure that maxSize leads to an error if exceeded. // Note that this uses a relatively non-sensical value of maxSize which
What do you think about panicking if targetSize > maxSize && maxSize > 0 ? Just in case we ever send a nonsensical value that way.
ajwerner
commented
Jan 30, 2020
Contributor
Author
ajwerner
left a comment
There was a problem hiding this comment.
TFTR!
bors r=itsbilal
Reviewable status:
pkg/ccl/storageccl/export_test.go, line 359 at r2 (raw file):
Previously, itsbilal (Bilal Akhtar) wrote…
What do you think about panicking if
targetSize > maxSize && maxSize > 0? Just in case we ever send a nonsensical value that way.
I considered it. The two reasons I didn't do that were:
- testing - it's handy here
- these parameters are going to generally controllable with cluster settings. We could add sanity checking there but I'd generally much prefer an error than a panic.
Contributor
Build failed (retrying...) |
craig bot
pushed a commit
that referenced
this pull request
Jan 30, 2020
44489: sql: bugfixes around writing the old primary key in pk changes r=rohany a=rohany This PR fixes two bugs: * The logic for when to rewrite the old primary key was broken resulting in the old primary key not being rewritten in many cases. * The old primary key being created was not properly dealing with its dangling interleave information. This PR makes the design decision to not re-interleave the copy of the old primary index if it was interleaved. Release note: None 44551: jobs: always set start time of a job r=spaskob a=spaskob When starting a job via CreateAndStartJob if making the job started fails the job will stil be in system.jobs and can be adopted by another node later but the started time will be 0 in this case. We add a check and set it if necessary. Release note: none. 44553: engine: redefine targetSize and add maxSize to ExportToSst r=itsbilal a=ajwerner This PR is a follow-up of work from #44440 motivated by problems unearthed while typing #44482. The PR comes in two commits: 1) Re-define `targetSize` from being a target below which most requests would remain to being the size above which the export stops. 2) Add a `maxSize` parameter above which the `ExportToSst` call will fail. See the individual commits for more details. Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu> Co-authored-by: Spas Bojanov <spas@cockroachlabs.com> Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Contributor
Build succeeded |
ajwerner
pushed a commit
to ajwerner/cockroach
that referenced
this pull request
Jan 31, 2020
In cockroachdb#44553 I used the wrong format specifier for an int64_t in libroach. While it did not break the build it seems to break something for developers using their local compiler (MacOS?). Release note: None
craig bot
pushed a commit
that referenced
this pull request
Jan 31, 2020
44583: libroach: fix format specifier r=ajwerner a=ajwerner In #44553 I used the wrong format specifier for an int64_t in libroach. While it did not break the build it seems to break something for developers using their local compiler (MacOS?). Release note: None Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.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 is a follow-up of work from #44440 motivated by problems unearthed while typing #44482. The PR comes in two commits:
targetSizefrom being a target below which most requests would remain to being the size above which the export stops.maxSizeparameter above which theExportToSstcall will fail.See the individual commits for more details.