Remove unused extents from tablet locator - #6498
Conversation
| lastTabletRow.append(new byte[] {'<'}, 0, 1); | ||
| } | ||
|
|
||
| private void onExtentRemoval(KeyExtent extent, TabletLocation location, RemovalCause cause) { |
There was a problem hiding this comment.
This will be invoked asynchronously which could allow the extent to be accessed after an eviction decision has already been made. Looking at the docs, I wonder if we should use evictionListener instead of removalListener on the cache for synchronous notification.
There was a problem hiding this comment.
I think you're right. Switched to evictionListener in 6a59935
|
|
||
| if (entry != null) { | ||
| KeyExtent ke = entry.getValue().tablet_extent; | ||
| TabletLocation location = extentCache.getIfPresent(entry.getValue().tablet_extent); |
There was a problem hiding this comment.
This code may not need to change if instead we synchronize on an object in this method and in the evictionListener to ensure that an eviction and lookup are not happening concurrently. I have not looked at other places in this class where extentCache lookups are done, but we would want to protect those as well.
Could use Guava's Striped class,Striped<Lock> specifically, for more granular locking on the extent.
dlmarion
left a comment
dlmarion
left a comment
There was a problem hiding this comment.
I think we may want synchronous notification and locking to remove the chance of a race condition between expiration and lookup.
| this.lockChecker = tslc; | ||
|
|
||
| extentCache = Caffeine.newBuilder().expireAfterAccess(CACHE_EXPIRATION) | ||
| .scheduler(Scheduler.systemScheduler()).evictionListener(this::onExtentEviction).build(); |
There was a problem hiding this comment.
Does this introduce deadlock? evictionlistener runs sync on teh thread doing the cache operation, and the locateTabletInCache is called under the rlock from binmutations, binranges, etc. So if getIfPresent can run the listener in line on a thread that holds the read lock the write lock will block forever, no?
Could the listener just enqueue the evicted extent and let processInvalidated drain it under the write lock it already holds?
I'm not sure I'm right about the deadlock though.....
There was a problem hiding this comment.
I think you might be right. Looking into a solution now.
3 participants
fixes #6169
Adds a cache to the online tablet locator that expires tablet extents after 10 minutes. Expired locations are reloaded from metadata when needed.
This reduces client memory usage.