[improve][broker]Reduce the lock range of SimpleCache to enhance performance

[poorbarcode]

Motivation

#23062 improved the behaviour of initialising the Transaction Buffer: Synchronise the reader creation, read loop and the following process on its result. Maintain only one reader for each namespace. The reader is now not closed unless there is no snapshot read request in 1 minute.

However, SimpleCache is a global lock, and the lock force is too strong, causing all pulsar-transaction-snapshot-recover threads to get stuck in the creation of the first reader. When system resources are insufficient, the problem will be magnified infinitely

---

Q1: The creation of the reader that reads __transaction_buffer_snapshot and the reading of existing messages are both executed synchronously. Is it necessary to change it to be completed asynchronously?

A1: It is not necessary, since the initialisation of all TransactionBuffers under the same namespace requires waiting for the reader to complete the processing of all messages, whether it is asynchronous or not is not important

Q2. Will these synchronisation operations cause the thread pulsar-transaction-snapshot-recover to get stuck and affect other functions

A2: No. The function of this thread is quite simple: 1. Initialise Transaction Buffer. 2. After the Transaction Buffer recovery is completed, handle the accumulated transaction write operations (subsequent writes will no longer use this thread).
So the stuck functions actually all need to wait for the reader's messages to be processed, and thus, no other impacts have been caused

Modifications

No longer wait for the initialisation of the reader to complete within the lock code block

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: x

[codelipenghui]

@poorbarcode Is it better just fix SimpleCache so get() does not run valueSupplier under a global synchronized lock? It will avoids per-thread TableView growth/leak risk.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.72%. Comparing base (74f4e5a) to head (037796c).
⚠️ Report is 49 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #25293      +/-   ##
============================================
- Coverage     72.73%   72.72%   -0.02%     
+ Complexity    34264    33886     -378     
============================================
  Files          1954     1954              
  Lines        154792   154875      +83     
  Branches      17731    17741      +10     
============================================
+ Hits         112586   112627      +41     
- Misses        33170    33206      +36     
- Partials       9036     9042       +6     

Flag	Coverage Δ
inttests	25.68% <62.06%> (-0.23%)	⬇️
systests	22.56% <0.00%> (+0.08%)	⬆️
unittests	73.71% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java	82.90% <100.00%> (+0.67%)	⬆️
...r/service/SystemTopicTxnBufferSnapshotService.java	81.57% <100.00%> (+1.29%)	⬆️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java	84.78% <100.00%> (+0.51%)	⬆️
...r/impl/SnapshotSegmentAbortedTxnProcessorImpl.java	81.58% <100.00%> (+0.38%)	⬆️

... and 87 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Denovo1998]

Can we add targeted tests for the new future lifecycle here?

The risks in this change lie not in the expected path, but in the ownership and expiration transitions:

• the cache entry is removed or expired before the future completes
• the future completes successfully after expiration and should close the reader exactly once
• the future completes exceptionally and should not try to close anything

[lhotari]

Please check the comment about using ConcurrentHasMap and moving locking to ExpirableValue class to avoid race conditions with expiration.

[Copilot]

Pull request overview

This PR reduces the lock contention in SimpleCache by moving the blocking reader creation out of the synchronized block. Previously, the get() method held the global lock while synchronously creating and waiting for a reader, which blocked all other threads. Now, the cache stores CompletableFuture<Reader<T>> instead of Reader<T>, and the lock is only held to insert/retrieve the future — the actual wait() call happens outside the lock.

Changes:

• SimpleCache gains a new getWithCacheInfo() method that exposes the ExpirableValue wrapper, allowing callers to update the deadline after async operations complete outside the lock.
• TableView now caches CompletableFuture<Reader<T>> and waits on the future outside the synchronized block, with updated expiration logic to handle incomplete futures.
• A new test TableViewTest.testFailedCreateReader validates behavior when reader creation fails.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
SimpleCache.java	Adds getWithCacheInfo() method and makes ExpirableValue and its members public.
TableView.java	Changes cache type to CompletableFuture<Reader<T>>, moves blocking wait outside the lock, adds expiration handling for futures, and extracts closeReader() helper.
TableViewTest.java	New test for failed reader creation scenario.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lhotari]

Please fix the resource leak that exists in tableViewThreadLocal. Fixing it is important for our CI. Test runs become slow, flaky, or fail (contribute to OOME) when resources are leaked.

[poorbarcode]

Please fix the resource leak that exists in tableViewThreadLocal. Fixing it is important for our CI. Test runs become slow, flaky, or fail (contribute to OOME) when resources are leaked.

I have already responded to your concern here. Please take a look first #25293 (comment)

Answered his question and pinged him in Slack, but he did not reply for a long time

[lhotari]

Please fix the resource leak that exists in tableViewThreadLocal. Fixing it is important for our CI. Test runs become slow, flaky, or fail (contribute to OOME) when resources are leaked.

I have already responded to your concern here. Please take a look first #25293 (comment)

@poorbarcode I'll fix that issue separately.

[poorbarcode]

@lhotari Could you point to the exact test method?

[lhotari]

@lhotari Could you point to the exact test method?

I took a closer look and in this case, it happens to be fine since TableView and SimpleCache don't keep state by starting their own threads. The allocated resources such as the Reader will get closed when the client closes. This is fine.