backupccl: backup correctly tries reading in from base directory if l… #77315
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 |
|
Didn't do too much testing since it was getting late, but basically I just went with the first easier solution (where we just always try reading from base directory on error) just to confirm that it was indeed the problem. Just running a few tests it seemed like it didn't break anything and the original issue seemed fixed. But i'll give it some more testing tomorrow and figure out what solution we want to go with. |
DarrylWong
force-pushed
the
latest-checkpoint-bug
branch
2 times, most recently
from
de639be to
e8a56ac
Compare
DarrylWong
requested review from
a team,
dt and
adityamaru
and removed request for
a team and
dt
adityamaru
reviewed
Mar 8, 2022
| latestFile, err = exportStore.ReadFile(ctx, latestFileName) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "Latest file could not be read in base or metadata directory") |
There was a problem hiding this comment.
Wasn't 100% sure what you meant by this, did you mean in place of "latest file" or the "metadata directory"
There was a problem hiding this comment.
ahh just change Latest to LATEST since thats the sentinel file name we use.
DarrylWong
force-pushed
the
latest-checkpoint-bug
branch
2 times, most recently
from
bbd7020 to
29ae66d
Compare
adityamaru
approved these changes
Mar 10, 2022
DarrylWong
force-pushed
the
latest-checkpoint-bug
branch
from
29ae66d to
1bff87f
Compare
…atest/checkpoint files aren't found Before, we only tried reading from the base directory if we caught a ErrFileDoesNotExist error. However this does not account for the potential error thrown when the progress/latest directories don't exist. This changes it so we now correctly retry reading from the base directory. We also put the latest directory inside of a metadata directory, in order to avoid any potential conflicts with there being a latest file and latest directory in the same base directory. Also wraps errors in findLatestFile and readLatestCheckpointFile for more clarity when both base and latest/progress directories fail to read. Fixes cockroachdb#77312 Release justification: Bug fix for release blocker Release note: none
DarrylWong
force-pushed
the
latest-checkpoint-bug
branch
from
1bff87f to
c9e9855
Compare
|
bors r+ |
|
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.
…atest/checkpoint files aren't found
Before, we only tried reading from the base directory if we caught a ErrFileDoesNotExist error. However
this does not account for the potential error thrown when the progress/latest directories don't exist.
This changes it so we now correctly retry reading from the base directory.
We also put the latest directory inside of a metadata directory, in order to avoid any potential
conflicts with there being a latest file and latest directory in the same base directory.
Also wraps errors in findLatestFile and readLatestCheckpointFile for more clarity when both base and
latest/progress directories fail to read.
Fixes #77312
Epic CRDB-7586
Release justification: Low risk bug fix
Release note: none