Skip to content

HBASE-30285 BucketCache shutdown/disable does not release backingMap-owned BucketEntry references#8469

Open
mwkang wants to merge 2 commits into
apache:masterfrom
mwkang:HBASE-30285
Open

HBASE-30285 BucketCache shutdown/disable does not release backingMap-owned BucketEntry references#8469
mwkang wants to merge 2 commits into
apache:masterfrom
mwkang:HBASE-30285

Conversation

@mwkang

@mwkang mwkang commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

See HBASE-30285.

BucketCache.backingMap owns the initial reference of each BucketEntry.
The existing shutdown and disable paths discard map entries without releasing these references, so their reference counts may never reach zero.

This change:

  • disables the cache and interrupts writer threads before cleanup
  • waits for in-flight writers before releasing backingMap entries
  • stops the cache persister before final serialization
  • persists metadata only for explicit persistent-cache shutdown
  • releases each backingMap owner reference under its offset write lock
  • preserves references held by in-flight callers until those callers release them
  • runs cleanup on a one-shot thread to avoid writer self-join and read-to-write lock upgrade deadlocks
  • removes corrupt persistent-cache entries through blockEvicted, preserving file and region cache metadata
  • does not add synchronization to the normal cache read, write, or writer queue paths

The tests cover:

  • non-persistent shutdown with a retained caller reference
  • cache disable after repeated IO errors
  • entries published by an in-flight writer
  • concurrent and repeated shutdown
  • persistent-cache serialization and recovery
  • corrupt persistent-cache reads through direct and reference-file lookups

@liuxiaocs7

Copy link
Copy Markdown
Member

We’d better keep the Jira title and commit message consistent, thanks!

@mwkang mwkang changed the title HBASE-30285 Release BucketEntry references when BucketCache shuts down HBASE-30285 BucketCache shutdown/disable does not release backingMap-owned BucketEntry references Jul 13, 2026

@wchevreuil wchevreuil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting this fix. I have some concerns about this, however.

Considering we call BucketCache.shutdown normally when stopping a RS process, the reference count leak here wouldn't be a big problem (the jvm process will be over soon). The introduction of locks on a hot code path, though, could have a performance impact.

Have we done any performance benchmark comparison for validating this change is not introducing a noticeable performance impact?

// the cache map state might differ from the actual cache. If we reach this block,
// we should remove the cache key entry from the backing map
backingMap.remove(key);
fileNotFullyCached(key, bucketEntry);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are we tracking the file as not fully cached now? This can affect the cache aware load balancer proper functioning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. In the revised patch, the failed-read path now releases the offset read lock first, finds the actual key stored in backingMap, and removes the entry under its offset write lock.

After a successful conditional removal, it calls blockEvicted(cacheKey, bucketEntry, true, false), which invokes fileNotFullyCached for persistent IO engines. This keeps the file and region cache metadata used by the cache-aware load balancer consistent.

I also added regression tests for both direct and reference-file lookups.

@mwkang

mwkang commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

The benchmark concern was valid for the previous revision, which added lifecycle locking around cacheBlock and writer-queue publication.
That hot-path locking and the queue-polling change have been removed.

Compared with master, cacheBlock and the normal writer-queue path are unchanged.
The successful getBlock path adds no new lock or backingMap scan. In getBlock, the new scan and offset write-lock path runs only after an HBaseIOException. Shutdown/disable cleanup runs after the cache is disabled and acquires per-entry offset write locks there.
Therefore, I did not run a benchmark for this revision.

The leak is also reachable when IOEngine read, write, or sync errors persist beyond the configured tolerance duration: BucketCache disables itself while the RegionServer JVM continues running. It is therefore not limited to process shutdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants