Skip to content
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

Merged
merged 10 commits into from
Dec 20, 2023
Merged

Various fixes and improvements (main) #243

merged 10 commits into from
Dec 20, 2023

Conversation

alanking
Copy link
Contributor

Supersedes #236

Addresses #191, #229, #235, #237, #239, #241

All the new tests pass. Running full test suite now.

storage_tiering.hpp Outdated Show resolved Hide resolved
packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@korydraughn korydraughn left a 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.

README.md Outdated Show resolved Hide resolved
libirods_rule_engine_plugin-unified_storage_tiering.cpp Outdated Show resolved Hide resolved
@alanking
Copy link
Contributor Author

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.

storage_tiering.cpp Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@korydraughn
Copy link
Collaborator

If everything compiles and the tests pass, let's squash and get it merged.

storage_tiering.cpp Outdated Show resolved Hide resolved
@alanking
Copy link
Contributor Author

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.

@alanking
Copy link
Contributor Author

Tests passed.

@korydraughn
Copy link
Collaborator

Squash when ready.

@alanking
Copy link
Contributor Author

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.

@trel
Copy link
Member

trel commented Dec 19, 2023

oh, the test itself is doing that? interesting.

@alanking
Copy link
Contributor Author

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.

@alanking
Copy link
Contributor Author

Well... test_plugin_unified_storage_tiering.TestStorageTieringContinueInxMigration.test_put_object_limit_lt failed, but succeeded after running again. Might be non-deterministic. Trying a few more times to see what kind of results I get.

@alanking
Copy link
Contributor Author

alanking commented Dec 19, 2023

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.

@alanking
Copy link
Contributor Author

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.
Copy link
Collaborator

@korydraughn korydraughn left a 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.

@alanking alanking merged commit 6f38236 into irods:main Dec 20, 2023
1 check failed
@alanking alanking deleted the 233.m branch December 20, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants