Copilot commented on code in PR #10633:
URL: https://github.com/apache/ozone/pull/10633#discussion_r3559001529
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -188,23 +192,39 @@ protected List<OmKeyLocationInfo> allocateBlock(
UserInfo userInfo, OzoneManager ozoneManager)
throws IOException {
final long scmBlockSize = ozoneManager.getScmBlockSize();
+ final KeyManager keyManager = ozoneManager.getKeyManager();
int dataGroupSize = replicationConfig instanceof ECReplicationConfig
? ((ECReplicationConfig) replicationConfig).getData() : 1;
final int numBlocks = (int)
Math.min(ozoneManager.getPreallocateBlocksMax(),
(requestedSize - 1) / (scmBlockSize * dataGroupSize) + 1);
- String clientMachine = "";
- if (shouldSortDatanodes) {
- clientMachine = userInfo.getRemoteAddress();
+ final String scmClientMachine;
+ final String omClientMachine;
+ final Map<List<DatanodeDetails>, List<? extends DatanodeDetails>>
sortedByNodes;
+ final NetworkTopology clusterMap = shouldSortDatanodes
+ && keyManager.isSortDatanodesForWriteEnabled()
+ ? ozoneManager.getClusterMapAllowNull() : null;
+ if (!shouldSortDatanodes) {
+ scmClientMachine = "";
+ omClientMachine = "";
+ sortedByNodes = null;
+ } else if (clusterMap != null) {
Review Comment:
When OM-side write sorting is enabled and the topology is available, this
branch will attempt to sort even if `userInfo.getRemoteAddress()` is empty.
That causes `KeyManager.sortDatanodesForWrite(...)` to throw (it requires a
non-empty clientMachine), which can fail allocateBlock for requests that omit
the remoteAddress. Gate OM-side sorting on a non-empty remoteAddress so empty
clients keep the original pipeline order as intended.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -2204,32 +2201,85 @@ 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) {
+ Preconditions.checkArgument(!StringUtils.isEmpty(clientMachine),
clientMachine);
Review Comment:
`Preconditions.checkArgument(..., clientMachine)` uses the clientMachine
value as the error message. When the value is empty (the failure case), the
exception message is empty and not actionable. Use a clear message so failures
are diagnosable.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -1342,6 +1343,11 @@ public void setScmTopologyClient(
}
public NetworkTopology getClusterMap() {
+ return requireNonNull(getClusterMapAllowNull(),
+ "ScmBlockLocationClient must have been initialized already.");
Review Comment:
The null-check message here refers to `ScmBlockLocationClient`, but this
method is returning OM's cached topology from `ScmTopologyClient`. Updating the
message will make failures easier to understand.
--
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]
