-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various fixes and improvements (main) #243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Got a couple questions.
The whole test suite passed before these last few changes. The latest changes are only renaming a function (the project still successfully builds) and updating comments and the README. So, I'll re-run the test suite again when an actual behavioral change occurs. Maybe just in case right before merging. Also, sneaking #154 in, for kicks. |
If everything compiles and the tests pass, let's squash and get it merged. |
Gonna run tests one more time for sanity. Also, might yield to #234 to avoid dealing with the conflict in that branch but rather deal with it here in this branch. |
Tests passed. |
Squash when ready. |
One of the new tests added in #234 is failing with my changes because it is restaging an object to a higher tier (which is now disallowed). I will need to update the test. |
oh, the test itself is doing that? interesting. |
Had to force-push due to rebase, but only the latest commit reflects new changes. The aforementioned test now passes. I had to make sure that the spirit of the test was maintained and that it was not also duplicating a test that I added in this PR. I think the needle has been threaded. Running all the tests once more. |
Well... |
I tried running the test 10 times in a row. The first 5 times it passed with similar times (~130 seconds). The 6th time has been running for over 20 minutes and I'm pretty sure it is never going to pass based on the assertion that it's stuck on. It appears that it tiered out 4 objects and stopped, not scheduling any more to tier out (there are 256 objects that need to be tiered out). I'm going to try this again without my commits to see if anything changes. |
I observed similar behavior with the commit upon which these changes are based, so I filed an issue for that test here: #246 Will await approval / confirmation to move forward before merging, just in case. |
Also removes the sample_configuration.json file as this is documented in the README and elsewhere.
This commit adds tests for restaging/tiering out to resources which already have a good replica. The result should be no data movement, and updated metadata to reflect the "tracked" replica.
When tiering out or restaging, a call to the replication API is made. This commit checks to see whether a good replica already exists in the hierarchy and skips the replication if so. This is required because good replicas are not allowed to be overwritten at the risk of losing data.
When a replica on a tier which is lower than the minimum restage tier is scheduled for restaging, it should not move to the minimum restage tier because that would entail restaging to a higher (slower) tier which defeats the purpose of a restage.
This commit gets the tier for the source resource and minimum restage tier and compares them before attempting a restage. If the source resource is in a tier that is lower than the minimum restage tier, the data migration for the restage should be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pound/Merge when ready.
Supersedes #236
Addresses #191, #229, #235, #237, #239, #241
All the new tests pass. Running full test suite now.