Disable invalidation time #18640
Disable invalidation time #18640
Conversation
core/server/master/src/main/java/alluxio/master/file/meta/UfsSyncPathCache.java
Outdated
Show resolved
Hide resolved
core/server/master/src/test/java/alluxio/master/file/meta/InvalidationSyncCacheTest.java
Outdated
Show resolved
Hide resolved
|
@apc999 @elega @jiacheliu3 Plz take a look as well |
ssyssy
force-pushed
the
disable-invalidation-time
branch
from
49d37a1 to
95fde63
Compare
There was a problem hiding this comment.
Since you can';t use JIRA link in this PR in open source, create a Github issue pointing to the master-2.x branch with an example jstack showing an example stacktrace of the loop. Then in the code comment, leave a link to that issue.
Also I feel we should keep the UT class, because the notifySyncedPath() call is still in the code. It's just we removed its callers. I prefer we just leave a comment in the UT code saying the only caller will be a cmdline manual call, and nothing else. In that case, if one day (or later) we figure this invalidation time feature should be back, the UT is there. Anyways, this is just a recommendation and not a very strong opinion.
Other than that, the code change LGTM so I will give the stamp beforehand. Trust you guys will handle the comments. Tests all pass and I'm not concerned so far. Let's see if your manual tests validate this fix.
Thanks a lot for the work!
| // We decide to stop updating invalidation time for paths in order to avoid | ||
| // paths got evicted recursively. Stopping calling this onCacheEviction | ||
| // function will result in stopping updating invalidation time for all paths. | ||
| // Since no one is using this invalidation time feature, we decide to disable | ||
| // updating invalidation time. |
There was a problem hiding this comment.
| // We decide to stop updating invalidation time for paths in order to avoid | |
| // paths got evicted recursively. Stopping calling this onCacheEviction | |
| // function will result in stopping updating invalidation time for all paths. | |
| // Since no one is using this invalidation time feature, we decide to disable | |
| // updating invalidation time. | |
| // Due to issue https://github.com/Alluxio/alluxio/issues/blah, | |
| // we decide to stop updating invalidation time for paths in order to avoid | |
| // paths got evicted recursively. Stopping calling this onCacheEviction | |
| // function will result in stopping updating invalidation time for all paths. | |
| // Since no one is using this invalidation time feature, we decide to disable | |
| // updating invalidation time. |
| * Tests the {@link UfsSyncPathCache} using only validations and invalidations, | ||
| * and without using interval based syncing. |
There was a problem hiding this comment.
* Due to issue https://github.com/Alluxio/alluxio/issues/blah, we removed
* the actual callers of invalidation time.
* Tests the {@link UfsSyncPathCache} using only validations and invalidations,
* and without using interval based syncing.
|
@ssyssy Let's address Jiacheng's comment today so we have some buffer for the release |
ssyssy
force-pushed
the
disable-invalidation-time
branch
from
95fde63 to
b62e847
Compare
ssyssy
force-pushed
the
disable-invalidation-time
branch
from
b62e847 to
b417e2b
Compare
|
alluxio-bot, merge this please |
|
merge failed: |
|
alluxio-bot, merge this please |
### What changes are proposed in this pull request? Disable the onCacheEviction Handler. ### Why are the changes needed? To fix a potential cache eviction recursion issue. ### Does this PR introduce any user facing changes? No. pr-link: #18640 change-id: cid-3766cad94d9ffcc511aadcf5078920cee51b96f4
What changes are proposed in this pull request?
Disable the onCacheEviction Handler.
Why are the changes needed?
To fix a potential cache eviction recursion issue.
Does this PR introduce any user facing changes?
No.