Skip to content

Conversation

@peterxcli
Copy link
Member

@peterxcli peterxcli commented Oct 24, 2025

What changes were proposed in this pull request?

  • Introduced RetryRequestBatcher, a sliding-window planner that keeps failed writeChunk requests sorted by end offset, retains only the most recent putBlock offset, and produces an optimized retry plan (combined chunk list + putBlock flag).
image
  • Wired the batcher into BlockOutputStream: every outgoing writeChunk/putBlock updates the window, writeOnRetry now replays the optimized plan (piggybacking the final chunk when supported), and acknowledgements/clears shrink the window once putBlock succeeds.
  • Added TestRetryRequestBatcher to exercise the batching logic across basic, duplicate putBlock, acknowledgement, complex, and bookkeeping scenarios.

Benefit:

  • Shared setup: Every writeChunk/putBlock RPC now flows through RetryRequestBatcher. On the happy path we track each write’s end-offset and the latest putBlock offset. If an RPC fails, the window already knows exactly which buffers still need to be retried and in what order; when a putBlock succeeds, acknowledgeUpTo(flushPos) removes all requests the datanodes have committed.

  • Retry without piggyback:

    • Old sequence: writeOnRetry blindly replayed each allocated chunk, issuing a writeChunk, immediately followed by a standalone putBlock. That meant n failed chunks produced 2n retry RPCs, even if multiple writes could be coalesced before the next metadata update.
    • New sequence: we call retryRequestBatcher.optimizeForRetry(). This collapses all outstanding chunks into a single ordered list and keeps just the highest putBlock offset. The retry loop now issues each chunk exactly once and sends a single putBlock at the end. Result: fewer network round-trips, less checksum/compression work, and shorter retry latency.
  • Retry with piggyback enabled:

    • Before: we still replayed every chunk one-by-one, and each chunk triggered a piggybacked writeChunkAndPutBlock, so we ended up sending a putBlock for every chunk in the window.
    • After: we write the combined chunk list sequentially; when we reach the last outstanding chunk, we piggyback the final putBlock on that single RPC (writeChunkAndPutBlock). All preceding chunks are sent as plain writeChunk calls. Effectively we collapse the retries to “N chunk writes + 1 piggybacked flush” instead of “N piggybacked writes”, reducing both network chatter and datanode commit work while preserving the benefits of piggyback (no extra standalone putBlock).

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11043

How was this patch tested?

TestRetryRequestBatcher UT

@peterxcli peterxcli changed the title wip HDDS-11043. Explore client retry optimizations after write() and hsync() are desynced Oct 24, 2025
@peterxcli peterxcli marked this pull request as draft October 24, 2025 17:26
@peterxcli peterxcli force-pushed the hdds11043-explore-client-retry-optimizations-after-write-and-hsync-are-desynced branch from 3f6ba7e to 28e738b Compare November 6, 2025 10:45
… enhance clarity in handling putBlock operations. Introduced RetryChunkSelection for better management of chunk buffers during retries, and streamlined the optimization logic in RetryRequestBatcher.
@peterxcli peterxcli force-pushed the hdds11043-explore-client-retry-optimizations-after-write-and-hsync-are-desynced branch from 2623b35 to e522657 Compare November 7, 2025 16:09
@peterxcli peterxcli marked this pull request as ready for review November 7, 2025 17:30
@peterxcli
Copy link
Member Author

Hi @jojochuang and @smengcl — just checking in. If you have a moment, I’d love any initial thoughts on this. Thanks!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces RetryRequestBatcher, a sliding-window optimizer that reduces network overhead during retry operations by combining multiple failed writeChunk requests and consolidating putBlock metadata operations. The optimization is particularly effective when write() and hsync() operations are desynchronized, reducing retry latency by minimizing redundant RPCs.

  • Introduces intelligent retry request batching that combines consecutive writeChunk operations and retains only the latest putBlock metadata
  • Refactors BlockOutputStream.writeOnRetry() to leverage the optimizer for fewer, more efficient retry RPCs
  • Implements lazy memory cleanup strategy that balances performance with memory usage

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/RetryRequestBatcher.java New class implementing sliding-window retry optimization with efficient tracking, acknowledgement handling, and lazy memory cleanup
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestRetryRequestBatcher.java Comprehensive unit tests covering basic operations, multi-chunk scenarios, acknowledgements, edge cases, and complex failure scenarios
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java Integrates RetryRequestBatcher into write path, refactors writeOnRetry to use optimized retry plans, adds helper classes for retry chunk selection
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java Removes @VisibleForTesting annotation from handleWrite method, making it package-private for internal use

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

@peterxcli peterxcli requested a review from ChenSammi December 5, 2025 06:42
@adoroszlai
Copy link
Contributor

@jojochuang @smengcl please take a look

@github-actions
Copy link

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

@github-actions github-actions bot added the stale label Jan 10, 2026
@github-actions
Copy link

Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.

@github-actions github-actions bot closed this Jan 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants