storageccl: pull in fix for reading GCS files #24880
Merged
Conversation
This file contains 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
|
This change is |
|
Oops hold on. Don't review yet. |
The linked issue lists a commit attempting to fix occasional problems we see during large GCS reads. They haven't been able to repro it but think this will fix it. Pull in that update so we can run it for a few months for testing. See: googleapis/google-cloud-go#784 Release note: None
|
Ok ready. PTAL. |
|
But please also open an issue with milestone 2.1 to track updating google-cloud-go to a proper release! Reviewed 3 of 3 files at r1. Comments from Reviewable |
|
No plans to backport this until the goog folks tag it and we've tested it enough in our nightlies to think it works. I want it in here so we can get a few months of testing to tell them if the bug went away or not. |
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Apr 18, 2018
24880: storageccl: pull in fix for reading GCS files r=mjibson a=mjibson The linked issue lists a commit attempting to fix occasional problems we see during large GCS reads. They haven't been able to repro it but think this will fix it. Pull in that update so we can run it for a few months for testing. See: googleapis/google-cloud-go#784 Release note: None 24896: engine: find split keys in the first range of a partition r=tschottdorf,a-robinson,bdarnell,danhhz a=benesch This deserves a roachtest—roachmart should make sure it has the correct number of ranges after inserting data—but I wanted to get this out for review ASAP. --- MVCCFindSplitKey would previously fail to find any split keys in the first range of a partition. As a result, partitioned tables have been observed with multi-gigabyte ranges. This commit fixes the bug. Specifically, MVCCFindSplitKey was assuming that the start key of a range within a table was also the row prefix for the first row of data in the range. This does not hold true for the first range of a table or a partition of a table--that range begins at, for example, /Table/51, while the row begins at /Table/51/1/aardvark. The old code had a special case for the first range in a table, but not for the first range in a partition. (It predates partitioning.) Remove the need for special casing by actually looking in RocksDB to determine the row prefix for the first row of data rather than attempting to derive it from the range start key. This properly handles partitioning and is robust against future changes to range split boundaries. See the commit within for more details on the approach. Release note (bug fix): Ranges in partitioned tables now properly split to respect their configured maximum size. Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The linked issue lists a commit attempting to fix occasional problems
we see during large GCS reads. They haven't been able to repro it
but think this will fix it. Pull in that update so we can run it for
a few months for testing.
See: googleapis/google-cloud-go#784
Release note: None