Fix setTtl workflow and ttlbucket bugs #16933
Merged
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
|
Automated checks report:
Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks. |
lucyge2022
force-pushed
the
ttl-bugfix
branch
from
2a731f6 to
2d5ce5e
Compare
|
Automated checks report:
All checks passed! |
lucyge2022
force-pushed
the
ttl-bugfix
branch
from
2f22aeb to
84a1c9b
Compare
ssyssy
reviewed
Mar 1, 2023
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucket.java
Outdated
Show resolved
Hide resolved
tcrain
reviewed
Mar 1, 2023
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucketList.java
Show resolved
Hide resolved
tcrain
reviewed
Mar 1, 2023
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Outdated
Show resolved
Hide resolved
and inode should load on inodettlchecker processing and be double checked on expiry. 2. setttl cmd should not set recursively on a directory, causing massive unnecessary setattr on children while dir ttl already supercede children ttl. 3. any inode failed during inodettlchecker process should add back to ttlbuckets for retry 4. Master.TTLInodes metrics can just be aggregated at time of gauging instead of atomic counter as it is not accurate during concurrent modification to ttlbuckets. 5. Fix race condition in TtlBucketList.insert(inode) where it might add to a bucket already got removed by ttlchecker (ALLUXIO-2821) 6. ttlbuckets should be polled first in each ttlchecker round to avoid concurrent modification while processing 7. add test cases for respecting dir ttl over children ttl.
…sing of ttlbucketlist
…ing inodes forever
jiacheliu3
reviewed
Mar 7, 2023
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucket.java
Outdated
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucket.java
Show resolved
Hide resolved
absolutely, feel free to take a deeper look! As I would really like to be reminded of anything regression from other components in master / in general. |
jiacheliu3
reviewed
Mar 9, 2023
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Show resolved
Hide resolved
jiacheliu3
reviewed
Mar 9, 2023
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Outdated
Show resolved
Hide resolved
jiacheliu3
requested changes
Mar 10, 2023
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucket.java
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucket.java
Outdated
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/meta/TtlBucketList.java
Outdated
Show resolved
Hide resolved
.../test/java/alluxio/client/fs/concurrent/ConcurrentFileSystemMasterSetTtlIntegrationTest.java
Outdated
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Show resolved
Hide resolved
core/server/master/src/main/java/alluxio/master/file/InodeTtlChecker.java
Show resolved
Hide resolved
.../test/java/alluxio/client/fs/concurrent/ConcurrentFileSystemMasterSetTtlIntegrationTest.java
Outdated
Show resolved
Hide resolved
.../test/java/alluxio/client/fs/concurrent/ConcurrentFileSystemMasterSetTtlIntegrationTest.java
Show resolved
Hide resolved
…eSystemMasterSetTtlIntegrationTest.java Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>
…ucket.java Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>
…ucket.java Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>
jiacheliu3
approved these changes
Mar 12, 2023
|
alluxio-bot, merge this please |
jiacheliu3
pushed a commit
to jiacheliu3/alluxio
that referenced
this pull request
May 17, 2023
### What changes are proposed in this pull request? 1)setttl cmd on a big directory unnecessary setattr recursively, while dir ttl already supercedes children ttl. there's absolutely no reason to save ttl on children node individually. 2)ttlbucket unnecessarily saves Inode obj on heap unboundedly, where it could and should only save inodeid and load inode from inodestore to double check expiry during inodettlchecker processing. 3)fix race condition on ttlbucketlists from ttlchecker and insertion from foreground. 4)fix case where any expiration of inodes failed during ttlchecker it will never be retried again. ### Why are the changes needed? Reduce heavy GC when setttl on a huge folder / bug fixes as indicated above for ttl ### Does this PR introduce any user facing changes? yes. So now setTtl command on a dir will not set ttl attributes for all its children. pr-link: Alluxio#16933 change-id: cid-36ec524b2178220886cccb7a82de119bdcc8203f
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.
What changes are proposed in this pull request?
1)setttl cmd on a big directory unnecessary setattr recursively, while dir ttl already supercedes children ttl. there's absolutely no reason to save ttl on children node individually.
2)ttlbucket unnecessarily saves Inode obj on heap unboundedly, where it could and should only save inodeid and load inode from inodestore to double check expiry during inodettlchecker processing.
3)fix race condition on ttlbucketlists from ttlchecker and insertion from foreground.
4)fix case where any expiration of inodes failed during ttlchecker it will never be retried again.
Why are the changes needed?
Reduce heavy GC when setttl on a huge folder / bug fixes as indicated above for ttl
Does this PR introduce any user facing changes?
yes. So now setTtl command on a dir will not set ttl attributes for all its children.