verify previous path state before backup #4630
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
ryanfkeepers
had a problem deploying
to
Testing
GitHub Actions
Failure
| driveID1: { | ||
| "root": rootFolderPath1, | ||
| "folder": rootFolderPath1 + "/folder", | ||
| "folder2": rootFolderPath1 + "/folder", |
There was a problem hiding this comment.
could you please add another folder that doesn't appear in the result set and make sure it's marked as deleted
There was a problem hiding this comment.
Hmm, well, it's not marked as deleted. It's not included in the result set either, which I think is correct given the other results and full enumeration. I'll follow up on this in the next PR where we don't force a full enumeration and make sure it's handled the right way there.
There was a problem hiding this comment.
I'm not sure if that's correct behavior or not. It would depend on what other data is returned. If we don't return a flag saying the base shouldn't be merged (which we still need to remove at some point) or mark the collection as deleted then it'll be pulled forward
I know this PR train isn't trying to solve that problem anymore, but I'm concerned about losing track of this particular test. It's important to have because we should handle it properly eventually, I just don't know the best way to record it
There was a problem hiding this comment.
It's tracked on a test in the next PR.
| errCheck: assert.NoError, | ||
| }, | ||
| { | ||
| name: "DuplicatePreviousPaths_separateDrives", |
There was a problem hiding this comment.
could you please also add a test where one drive has duplicate paths and the other doesn't? We should only be dropping metadata info for the one that has the duplicate in the ideal case
There was a problem hiding this comment.
Going to skip this. Same reason as skipping the per-drive metadata removal; we won't be forcing full enumeration on the next PR, so it won't be handled this way.
There was a problem hiding this comment.
I think it's still worth adding a test for this so we can, at the very least, build on it later on. Adding it will help ensure we don't lose track of that case and that alerts act as expected in the next PR
There was a problem hiding this comment.
fwiw, there's already a test for that in the next PR
|
@ashmrtn ready for final review |
ryanfkeepers
had a problem deploying
to
Testing
GitHub Actions
Failure
| errCheck: assert.NoError, | ||
| }, | ||
| { | ||
| name: "DuplicatePreviousPaths_separateDrives", |
There was a problem hiding this comment.
I think it's still worth adding a test for this so we can, at the very least, build on it later on. Adding it will help ensure we don't lose track of that case and that alerts act as expected in the next PR
| driveID1: { | ||
| "root": rootFolderPath1, | ||
| "folder": rootFolderPath1 + "/folder", | ||
| "folder2": rootFolderPath1 + "/folder", |
There was a problem hiding this comment.
I'm not sure if that's correct behavior or not. It would depend on what other data is returned. If we don't return a flag saying the base shouldn't be merged (which we still need to remove at some point) or mark the collection as deleted then it'll be pulled forward
I know this PR train isn't trying to solve that problem anymore, but I'm concerned about losing track of this particular test. It's important to have because we should handle it properly eventually, I just don't know the best way to record it
Drive previous paths have a prior bug or condition that allows multiple previousPath metadata folder ID entries within a single drive point to the same previous path string. In these cases we need to treat the backup as having no prior metadata, and re-run the full delta query to correct the bad state.
fail the backup if it produces a previous path collision in drive metadata.
ryanfkeepers
force-pushed
the
catch-duplicate-prev-paths
branch
from
4229eac to
4014578
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
ryanfkeepers
had a problem deploying
to
Testing
GitHub Actions
Failure
Drive previous paths have a prior bug or condition that allows multiple previousPath metadata folder ID entries within a single drive point to the same previous path string.
In these cases we need to treat the backup as having no prior metadata, and re-run the full delta query to correct the bad state.
Does this PR need a docs update or release note?
Type of change
Test Plan