-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDDS-1802. Add Eviction policy for table cache. #1100
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
Conversation
One missing thing in this PR is on the restart, we should load up tables with cache, where cleanup policy is NEVER from DB. Will post a fix for that in next commit. |
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.
I understand why we added this policy-specific check here. However it is probably misplaced here. Your original solution to have two cache types with isExist
overloaded was probably better.
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.
Same here.. we should not have these checks here.
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.
The Cache doesn't know it is being invoked on flush.. so this check is misleading.
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.
It is caller responsibility to call this method after the flush. And right now this is called only after flushing to DB from OzoneManagerDoubleBuffer.
So, if it is NEVER, we will not do the cleanup.
This comment has been minimized.
This comment has been minimized.
Capturing our offline discussion. We discussed the following:
Anything I missed? |
Thank You @arp7 for the offline discussion. |
544f8fc
to
89648ea
Compare
This comment has been minimized.
This comment has been minimized.
89648ea
to
2dd7942
Compare
/retest |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@arp7 Thanks for the review. |
hadoop-hdds/common/src/main/java/org/apache/hadoop/utils/db/cache/CacheResult.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/utils/db/cache/TableCache.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/utils/db/cache/TableCacheImpl.java
Outdated
Show resolved
Hide resolved
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.
+1
Minor suggestions to rename an enum and a function. Looks great otherwise.
Thank You @arp7 for the review. |
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.
+1 pending CI. Thanks @bharatviswa504!
Thank You @arp7 for the review. |
💔 -1 overall
This message was automatically generated. |
… case sensitive. (apache#1100)
No description provided.