rich7420 opened a new pull request, #1560:
URL: https://github.com/apache/ratis/pull/1560

   ## What changes were proposed in this pull request?
   
   A data stream's `DataStreamMap` entry is added on `STREAM_HEADER`
   (`DataStreamManagement.computeDataStreamIfAbsent`) and removed by exactly 
two sites: the
   Raft log worker when the `DATASTREAM` log entry is linked 
(`SegmentedRaftLogWorker`), or
   `StreamInfo.cleanUp` on the error / channel-inactive path.
   
   On a **successful CLOSE**, `DataStreamManagement` drains `streams` and the 
`channels` entry
   but hands the `DataStreamMap` entry off to the log path — it does not remove 
it. If that
   `DATASTREAM` log entry is never written (commit not logged: abort, leader 
change, follower
   truncation), the entry is orphaned: the closed stream is no longer tracked 
by `streams` or
   `channels`, so channel-inactive cleanup cannot reclaim it — and since the 
client may keep the
   channel open (Ozone reuses it), channel-inactive may never fire. The orphaned
   `CompletableFuture<DataStream>` pins the Netty direct buffers, so direct 
memory grows under
   sustained streaming write. This matches the MAT analysis on RATIS-2213 / 
HDDS-11939.
   
   This PR adds a close-time backstop (`scheduleUnlinkedStreamCleanup`): after 
the data-stream
   request timeout, remove the `DataStreamMap` entry **iff it is still 
unlinked** and clean the
   `DataStream`. A non-null remove result means the log worker has not linked 
it, so the
   `DataStream` is not yet owned by the state machine and is safe to clean; if 
it was already
   linked, remove returns null and the backstop is a no-op.
   
   Open to feedback on the approach — in particular: reusing 
`raft.client.data-stream.request.timeout`
   as the "abandoned" threshold vs. a dedicated grace period, and whether a 
link-completion callback
   from the log worker would be preferable to a per-close scheduled task.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/RATIS-2213
   
   ## How was this patch tested?
   
   New unit test 
`TestDataStreamManagement#closedStreamLeaksDataStreamMapWhenNotLogged`, reusing
   the existing `writeCommandInvokesOnCommand` harness with an observable 
`DataStreamMap`. It drives
   a full `HEADER -> DATA -> DATA+CLOSE` stream, asserts the channel map is 
drained on close while the
   `DataStreamMap` entry remains, then asserts the entry is reclaimed after the 
request timeout. The
   test fails without the fix (`expected: <0> but was: <1>`) and passes with 
it. Full
   `TestDataStreamManagement` suite: 12/12 pass. 
`TestNettyDataStreamStarTopologyWithGrpcCluster`
   (real 3-node cluster, `write -> close -> link -> commit`): 14/14 pass, 
confirming the backstop
   no-ops on the happy path (a linked stream's `remove` returns null, so 
nothing is reaped).
   
   CI on fork: https://github.com/rich7420/ratis/actions/runs/32698135039
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to