HDDS-15059. Shift streaming write sortDatanodes logic to OM by chihsuan · Pull Request #10633 · apache/ozone

chihsuan · 2026-06-29T17:05:46Z

What changes were proposed?

Move streaming-write pipeline sorting from SCM to OM. OM already caches the cluster topology; making SCM repeat the sort on every allocateBlock adds work to the SCM hot path, especially when several OMs share one SCM.

The new ozone.om.block.write.sort.datanodes.enabled option is disabled by default, preserving the current SCM-side behavior. When enabled:

OM resolves RPC-deserialized pipeline nodes against its cached topology before sorting, then maps the order back to the original nodes.
The client is matched by IP or hostname. If it is empty or cannot be resolved, the original pipeline order is kept because the first node is the write primary.
Pipelines with the same nodes are sorted once per allocation request.
If the OM topology cache is unavailable, the client address is sent to SCM and SCM performs the sort.

SCM behavior is unchanged, so old OMs still get SCM-side sorting during a rolling upgrade. There is no Protobuf or RPC change.

JIRA

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

Testing

TestOMAllocateBlockRequest: configuration paths, SCM fallback, empty clients, pipeline ordering, and per-pipeline caching.
TestOMSortDatanodes: topology-aware ordering for RPC-deserialized nodes, IP/hostname matching, and unresolved clients.
Fork build-branch CI: https://github.com/chihsuan/ozone/actions/runs/28455187435

Generated-by: Claude Code (Claude Opus 4.8)

…lock

…comment

…eBlock

… client

peterxcli

@ivandika3 Is there any risk that a block ends up allocated on a suboptimal datanodes/pipeline because the OM cache topology is somehow stale?⁠

ivandika3 · 2026-07-02T01:36:19Z

@peterxcli thanks for checking.

Is there any risk that a block ends up allocated on a suboptimal datanodes/pipeline because the OM cache topology is somehow stale?⁠

Should have some effect on performance but not correctness, since Streaming Write Pipeline should be able to pick an arbitrary topology. The the data path (streaming WriteChunk data) can be sent to any primary (first node) is separated from the metadata path (PutBlock) which will be sent to the DN leader. The impact of suboptimal Streaming write pipeline topology should be worse write latency (e.g. if the topology picks the furthest node as the primary node). But this possible performance penalty also apply to read path (i.e. where the further node is read first) so I think it should be acceptable. Please let me know if I miss something.

@chihsuan Thanks for the patch, I'll review this soon.

szetszwo · 2026-07-02T18:54:50Z

... We can do the sorting on the OM instead of SCM to reduce SCM RPC processing time.

@ivandika3 , @chihsuan , Would this change improve the overall performance?

OM is supposed to be more CPU demanding than SCM. So, we probably want to save the CPU cycles in OM. What do you think?

ivandika3 · 2026-07-03T06:05:13Z

@szetszwo Thanks for checking this out.

The reason I raised for this is to follow the HDDS-9343 which should make the sort datanodes logic done in OM.

Would this change improve the overall performance?

I have not measured the overall performance difference yet, but for a single OM and single OM service, the performance improvement (if any) might not be that much.

OM is supposed to be more CPU demanding than SCM. So, we probably want to save the CPU cycles in OM. What do you think?

That is a good point. However, in our cluster we have multiple OM services that points to a single SCM service, so I think SCM service resources should be protected more since it can be the bottleneck as more OM services point to the same SCM service. Additionally, ideally SCM should spend most of its times on background services (processing heartbeat reports, etc) and therefore should spend as little time as possible in the user foreground processing (i.e. allocating blocks, fetching read pipelines, etc) which includes sort datanodes.

Please let me know what you think. This is not really a critical issue so I think we can defer it if you don't see any reason to support it now.

szetszwo · 2026-07-03T15:44:45Z

... in our cluster we have multiple OM services ...

Good to know that you are running with multiple OM services!

... This is not really a critical issue so I think we can defer it if you don't see any reason to support it now.

How about making it configurable?

ivandika3 · 2026-07-04T04:20:31Z

How about making it configurable?

Yes, making it configurable is a good idea.

chihsuan · 2026-07-04T07:03:24Z

Thanks @szetszwo and @ivandika3 for the discussion.

I'll add an OM-side boolean (e.g. ozone.om.block.write.sort.datanodes.enabled, name open to bikeshed), defaulting to false (SCM sorts, legacy behavior) so the default doesn't add OM CPU. Set true to sort on OM instead, for multiple OMs behind one SCM.

Does false sound like the right default, or would you prefer true?

ivandika3 · 2026-07-04T07:26:23Z

Does false sound like the right default, or would you prefer true?

@chihsuan Thanks for the follow up, let's set the default to false to preserve the current behavior. We can remove this configuration in the future if there is a significant performance improvement.

chihsuan · 2026-07-06T13:38:25Z

Hi @ivandika3 I've added an OM-side toggle in ec36f7d (default false, so SCM keeps doing the sort; set true to sort on OM). The change is pushed. Reviews welcome whenever you get a chance, thanks! 🙏

ivandika3

Thanks @chihsuan for the update, left some more comments.

ivandika3 · 2026-07-08T01:59:29Z

+    final Map<List<DatanodeDetails>, List<? extends DatanodeDetails>> sortedByNodes =
+        sortOnOm ? new HashMap<>() : null;


In KeyManagerImpl#sortDatanodes, this logic uses Map<Set<String>< List<? extends DatanodeDetails>> but in here it is Map<List<DatanodeDetails>, List<? extends DatanodeDetails>>. Any reason for this? If so, can we standardize the implementation?

I think there is no need to get the use the DatanodeDetails::getUuidString in KeyManagerImpl#sortedByNodes.

Also this comment is not entirely accurate

// Cache the sorted order per pipeline so blocks sharing a pipeline are
// sorted once (mirrors the read path's per-pipeline caching).

If there are two blocks with different pipelines, but consist of the same set of datanodes, they will share the same cache.

Any reason for this? If so, can we standardize the implementation?
I think there is no need to get the use the DatanodeDetails::getUuidString in KeyManagerImpl#sortedByNodes.

Agreed on both. Since DatanodeDetails#equals/hashCode are UUID-based, keying by the node list is already equivalent to keying by UUIDs, so the getUuidString -> Set<String> step is redundant.

I originally left the read path untouched to keep the change scoped to the write path, but since you raised it, I've dropped getUuidString and standardized both paths to key by the pipeline node list in 7d3bc3e. Thanks!

Also this comment is not entirely accurate

Good catch. Reworded to "blocks whose pipelines have the same datanodes are sorted once" in 5d67fdd

ivandika3 · 2026-07-08T02:17:48Z

+  /**
+   * Resolve a node to its canonical, topology-linked instance in the given
+   * cluster map, or return the input node if it is not in the map.
+   */
+  private Node toClusterMapNode(NetworkTopology clusterMap, Node node) {
+    final Node resolved = clusterMap.getNode(node.getNetworkFullPath());
+    return resolved != null ? resolved : node;
  }


Nit: The javadoc is kind of confusing with terms like "canonical", "topology-linked".

Also instead of returning the unresolved node, should we return null instead and skip the whole sort by distance since it should fail (e.g. the unresolved node should not have the correct ancestor information)? If so, then we can simply use thie NetworkTopologyImpl#getNode and we don't need this function.

Also I saw that before this toClusterMapNode we already resolve the client machine in getClientNode. If that's the case, is this still needed?

Nit: The javadoc is kind of confusing with terms like "canonical", "topology-linked".

Thanks, I've updated both javadocs to describe the lookup plainly and dropped "canonical". d310622

should we return null instead and skip the whole sort by distance since it should fail (e.g. the unresolved node should not have the correct ancestor information)?

That works for datanodes, which we already skip the sort if one is missing from the map, but not for the client. A client that isn't a datanode is never in the map, yet it still sorts fine because getOtherNode attaches it to its rack. Returning null would skip sorting for all external clients.

is this still needed?

Yes, getClientNode could return an RPC-deserialized node that isn't linked to the topology. But you're right it's better done inside getClientNode itself, so I moved the lookup there and removed toClusterMapNode in 356023d. Hope this makes it easier to read, thanks for the feedback!

…ring

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

chihsuan · 2026-07-10T13:28:38Z

  public NetworkTopology getClusterMap() {
-    return requireNonNull(cache.get(),
-        "ScmBlockLocationClient must have been initialized already.");
+    return cache.get();
  }


This is intentional and has already been handled one layer up.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

… match comment

Copilot

Pull request overview

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

chihsuan · 2026-07-10T13:33:31Z

Thanks for the detailed review! @szetszwo I merged the latest master to build on the #10701 (HDDS-15803) refactor and applied the proposed changes:

Added SCM fallback when the OM topology cache is unavailable.
Added the nullable topology accessor while preserving the strict OzoneManager#getClusterMap() contract.
Removed the NullPointerException catch from KeyManagerImpl.
Added a unit test for the fallback path.

The only difference from the suggested snippet is the condition for OM-side sorting: it checks that OM sorting is enabled and the topology is available. This preserves the configuration semantics. Otherwise, sorting falls back to SCM.

I've also addressed all the Copilot review comments.

When you have a chance, could you please take another look? Thank you! 🙇‍♂️

szetszwo

@chihsuan , thanks for the update! Please see the comments inlined.

szetszwo · 2026-07-10T19:58:47Z

-      final Map<Set<String>, List<? extends DatanodeDetails>> sortedPipelines = new HashMap<>();
+      final Map<PipelineID, List<? extends DatanodeDetails>> sortedPipelines = new HashMap<>();


Just a question: why change key from UUIDs to PipelineID?

One difference I can see is that the set of UUIDs (i.e. datanodes) can have multiple pipelines. In such case, we sort the same set of datanodes multiple times. Not sure if this case is common.

Good question! I switched it to PipelineID because Copilot flagged the new write-path cache for using an order-sensitive List key and aligned the read path to match for consistency.

One difference I can see is that the set of UUIDs (i.e. datanodes) can have multiple pipelines. In such case, we sort the same set of datanodes multiple times. Not sure if this case is common.

That's a fair point, and it changes my mind. The sort depends only on the datanode set, so keying by the set collapses them into a single sort, while PipelineID re-sorts each. It's never worse than PipelineID, and it's what the read path is already used on master. I've reverted both to the UUID set (7307ae2).

szetszwo · 2026-07-10T20:05:05Z

+        final List<DatanodeDetails> nodes = pipeline.getNodes();
+        final List<? extends DatanodeDetails> sorted = sortedByPipeline
+            .computeIfAbsent(pipeline.getId(),
+                id -> keyManager.sortDatanodesForWrite(nodes, omClientMachine));


We should also pass the clusterMap.

Good call, done in 59b6b3b

Copilot

Pull request overview

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

…path

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

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

chihsuan · 2026-07-11T07:55:04Z

Thanks @szetszwo I've pushed a few commits addressing comments (replied inline for details).

chihsuan added 7 commits June 29, 2026 21:54

HDDS-15059. Add OM-side sortDatanodesForWrite with latency metric

a767f7b

HDDS-15059. Sort write pipeline on OM instead of SCM during allocateB…

35ad85c

…lock

HDDS-15059. Address review: metric getter visibility and test helper …

c3ea84c

…comment

HDDS-15059. Guard empty client and cache per-pipeline sort in allocat…

fa5a466

…eBlock

HDDS-15059. Strengthen allocateBlock sort tests for reorder and empty…

5b19b72

… client

HDDS-15059. Match write-sort client by both hostname and IP

e29a899

HDDS-15059. Resolve write-sort pipeline nodes to OM cluster map

91bed08

chihsuan force-pushed the HDDS-15059 branch from 928a105 to 91bed08 Compare June 30, 2026 14:38

chihsuan added 3 commits June 30, 2026 23:13

HDDS-15059. Consolidate write-sort datanode tests

59f9c26

HDDS-15059. Read cluster map once per sort to avoid topology swap race

13f1e1a

HDDS-15059. Simplify write-sort client handling

5040d00

chihsuan marked this pull request as ready for review June 30, 2026 17:08

peterxcli reviewed Jul 1, 2026

View reviewed changes

ivandika3 requested a review from szetszwo July 2, 2026 01:36

chihsuan force-pushed the HDDS-15059 branch from 7077ea4 to 91f9933 Compare July 6, 2026 13:34

HDDS-15059. Make OM-side write sortDatanodes configurable (default off)

ec36f7d

chihsuan force-pushed the HDDS-15059 branch from 91f9933 to ec36f7d Compare July 6, 2026 13:35

jojochuang requested review from ashishkumar50 and ivandika3 July 6, 2026 16:24

ivandika3 reviewed Jul 8, 2026

View reviewed changes

chihsuan added 2 commits July 8, 2026 21:30

HDDS-15059. Key read-sort cache by pipeline nodes, dropping getUuidSt…

7d3bc3e

…ring

HDDS-15059. Clarify write-sort cache comment (keyed by pipeline nodes)

5d67fdd

Copilot started reviewing on behalf of chihsuan July 10, 2026 13:12 View session

HDDS-15059. Key read-path sort cache by pipeline id for consistency

3b9e456

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 10, 2026 13:16

Copilot started reviewing on behalf of chihsuan July 10, 2026 13:17 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread ...manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMAllocateBlockRequest.java

HDDS-15059. Assert write order via getNodesInOrder and clarify client…

49f4b02

… match comment

Copilot AI review requested due to automatic review settings July 10, 2026 13:26

Copilot started reviewing on behalf of chihsuan July 10, 2026 13:26 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

szetszwo reviewed Jul 11, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 11, 2026 06:50

Copilot started reviewing on behalf of chihsuan July 11, 2026 06:50 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

chihsuan added 2 commits July 11, 2026 15:05

HDDS-15059. Pass OM cluster map into sortDatanodesForWrite

59b6b3b

HDDS-15059. Key write-sort cache by datanode UUID set, matching read …

7307ae2

…path

Copilot AI review requested due to automatic review settings July 11, 2026 07:06

chihsuan force-pushed the HDDS-15059 branch from 34bcfa1 to 7307ae2 Compare July 11, 2026 07:06

Copilot started reviewing on behalf of chihsuan July 11, 2026 07:06 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

Comment thread ...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

HDDS-15059. Cache datanode sort only when an actual sort happens

de7d86c

Copilot AI review requested due to automatic review settings July 11, 2026 07:32

Copilot started reviewing on behalf of chihsuan July 11, 2026 07:32 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

Comment thread hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Comment thread hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java

HDDS-15059. Null-check clusterMap and document instance-return contract

93b0da9

Copilot AI review requested due to automatic review settings July 11, 2026 07:44

Copilot started reviewing on behalf of chihsuan July 11, 2026 07:44 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

		final Map<List<DatanodeDetails>, List<? extends DatanodeDetails>> sortedByNodes =
		sortOnOm ? new HashMap<>() : null;

		final Map<Set<String>, List<? extends DatanodeDetails>> sortedPipelines = new HashMap<>();
		final Map<PipelineID, List<? extends DatanodeDetails>> sortedPipelines = new HashMap<>();

Uh oh!

Conversation

chihsuan commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed?

JIRA

Testing

Uh oh!

peterxcli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivandika3 commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Jul 2, 2026

Uh oh!

ivandika3 commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Jul 3, 2026

Uh oh!

ivandika3 commented Jul 4, 2026

Uh oh!

chihsuan commented Jul 4, 2026

Uh oh!

ivandika3 commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chihsuan commented Jul 6, 2026

Uh oh!

ivandika3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivandika3 Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihsuan Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

chihsuan commented Jul 10, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

chihsuan commented Jun 29, 2026 •

edited

Loading

peterxcli left a comment •

edited

Loading

ivandika3 commented Jul 2, 2026 •

edited

Loading

ivandika3 commented Jul 3, 2026 •

edited

Loading

ivandika3 commented Jul 4, 2026 •

edited

Loading

ivandika3 Jul 8, 2026 •

edited

Loading

chihsuan Jul 8, 2026 •

edited

Loading