Copilot commented on code in PR #10633:
URL: https://github.com/apache/ozone/pull/10633#discussion_r3563591541
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -2204,32 +2209,86 @@ private void sortDatanodes(String clientMachine,
List<OmKeyInfo> keyInfos) {
@VisibleForTesting
public List<? extends DatanodeDetails> sortDatanodes(List<? extends
DatanodeDetails> nodes,
String clientMachine) {
- final Node client = getClientNode(clientMachine, nodes);
- return ozoneManager.getClusterMap()
- .sortByDistanceCost(client, nodes, nodes.size());
+ final NetworkTopology clusterMap = ozoneManager.getClusterMap();
+ final Node client = getClientNode(clientMachine, nodes, clusterMap);
+ return clusterMap.sortByDistanceCost(client, nodes, nodes.size());
+ }
+
+ @Override
+ public List<? extends DatanodeDetails> sortDatanodesForWrite(
+ List<? extends DatanodeDetails> nodes, String clientMachine,
NetworkTopology clusterMap) {
+ Preconditions.checkArgument(!StringUtils.isEmpty(clientMachine),
+ "clientMachine is empty");
+ return captureLatencyNs(
Review Comment:
sortDatanodesForWrite dereferences clusterMap (getNode/sortByDistanceCost)
but doesn't validate it. Since this is a new public KeyManager API, passing a
null clusterMap will currently fail with an unhelpful NPE; add an explicit null
check so failures are clear and immediate.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java:
##########
@@ -363,4 +365,25 @@ DeleteKeysResult getPendingDeletionSubFiles(long volumeId,
long bucketId, OmKeyI
* @return BackgroundService
*/
CompactionService getCompactionService();
+
+ /**
+ * Sort the datanodes of a write pipeline by network-topology distance to the
+ * client, using OM's locally cached cluster map. Unlike the read-path sort,
+ * the original order is preserved when the client cannot be resolved,
because
+ * the first node is used as the streaming-write primary.
+ *
+ * @param nodes the pipeline nodes to sort
+ * @param clientMachine client address (IP or hostname)
+ * @param clusterMap OM's cached cluster map used to resolve topology
distance
+ * @return nodes sorted nearest-first, or the input order if client is
unresolved
+ */
Review Comment:
OMKeyRequest uses reference equality (sorted != nodes) to decide whether to
cache a sort result, so correctness relies on sortDatanodesForWrite returning
the original list instance when sorting is skipped (eg client unresolved /
topology stale). The current JavaDoc only promises preserving order, not
preserving the list instance, which makes the contract easy to break in future
implementations.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
