backupccl: fix progress updating test#58015
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Dec 21, 2020
Merged
Conversation
Member
|
This change is |
adityamaru
approved these changes
Dec 17, 2020
adityamaru
reviewed
Dec 17, 2020
| // produce. It merges contiguous ranges on the same node. | ||
| // It sorts ranges by start_key and counts the number of times the | ||
| // lease_holder changes by comparing against the previous row's lease_holder. | ||
| mergedRangeQuery := ` |
Contributor
There was a problem hiding this comment.
nice query, didn't even know you could do this kind of thing in SQL.
This test was previously flaky since it was assuming that backup would issue the same number of requests as spans issued. This assumption was incorrect, and fixing the progress accounting for backup revealed that this test was faulty. While planning the work distribution for backup worker nodes, PartitionSpans automatically merges spans that are co-located on the same node, therefore reducing the number of ExportRequests issued. Before this commit, we used to block on the ExportRequest responses. The blocking was triggered on a per-range-request level. However, progress is only updated when the processor-level batch request is returned. This meant that the test might block a batch request and therefore the progress of the job would be less than what the test expected. This is now fixed by adjusting the blocking mechanism and range counting to all be at the level of the merged ranges with which the backup processor operates. Release note: none
4612b53 to
8d44e48
Compare
Contributor
Author
|
TFTR! |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
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 test was previously flaky since it was assuming that backup would
issue the same number of requests as spans issued. This assumption was
incorrect, and fixing the progress accounting for backup revealed that
this test was faulty.
While planning the work distribution for backup worker nodes,
PartitionSpans automatically merges spans that are co-located on the
same node, therefore reducing the number of ExportRequests issued.
Before this commit, we used to block on the ExportRequest responses.
The blocking was triggered on a per-range-request level. However,
progress is only updated when the processor-level batch request is
returned. This meant that the test might block a batch request and
therefore the progress of the job would be less than what the test
expected.
This is now fixed by adjusting the blocking mechanism and range counting
to all be at the level of the merged ranges with which the backup
processor operates.
Fixes #57831.
This test would fail under stress pretty quickly (~40) -- I was able to get
over 600 runs without failure with this patch.
Release note: none