szetszwo commented on code in PR #10633:
URL: https://github.com/apache/ozone/pull/10633#discussion_r3545682763
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -190,25 +193,28 @@ protected List< OmKeyLocationInfo >
allocateBlock(ScmClient scmClient,
ReplicationConfig replicationConfig, ExcludeList excludeList,
long requestedSize, long scmBlockSize, int preallocateBlocksMax,
boolean grpcBlockTokenEnabled, String serviceID, OMMetrics omMetrics,
- boolean shouldSortDatanodes, UserInfo userInfo)
+ boolean shouldSortDatanodes, UserInfo userInfo, KeyManager keyManager)
throws IOException {
int dataGroupSize = replicationConfig instanceof ECReplicationConfig
? ((ECReplicationConfig) replicationConfig).getData() : 1;
int numBlocks = (int) Math.min(preallocateBlocksMax,
(requestedSize - 1) / (scmBlockSize * dataGroupSize) + 1);
- String clientMachine = "";
- if (shouldSortDatanodes) {
- clientMachine = userInfo.getRemoteAddress();
- }
+ final boolean sortOnOm =
+ shouldSortDatanodes && keyManager.isSortDatanodesForWriteEnabled();
+ // When SCM sorts, it still needs the real client address; the OM-sort path
+ // sends an empty address so SCM skips its sort.
+ final String scmClientMachine =
+ (shouldSortDatanodes && !sortOnOm) ? userInfo.getRemoteAddress() : "";
+ final String omClientMachine = sortOnOm ? userInfo.getRemoteAddress() : "";
Review Comment:
Let's separate the three different cases. It is easier to understand.
```java
final String scmClientMachine;
final String omClientMachine;
final Map<List<DatanodeDetails>, List<? extends DatanodeDetails>>
sortedByNodes;
if (!shouldSortDatanodes) {
// do not sort datanodes
scmClientMachine = "";
omClientMachine = "";
sortedByNodes = null;
} else if (keyManager.isSortDatanodesForWriteEnabled()) {
// sort datanodes in OM
scmClientMachine = "";
omClientMachine = userInfo.getRemoteAddress();
sortedByNodes = new HashMap<>();
} else {
// sort datanodes in SCM
scmClientMachine = userInfo.getRemoteAddress();
omClientMachine = "";
sortedByNodes = null;
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -218,13 +224,27 @@ protected List< OmKeyLocationInfo >
allocateBlock(ScmClient scmClient,
}
throw ex;
}
+ // Cache the sorted order by pipeline nodes so blocks whose pipelines have
+ // the same datanodes are sorted once (mirrors the read path's caching).
+ final Map<List<DatanodeDetails>, List<? extends DatanodeDetails>>
sortedByNodes =
+ sortOnOm ? new HashMap<>() : null;
for (AllocatedBlock allocatedBlock : allocatedBlocks) {
BlockID blockID = new BlockID(allocatedBlock.getBlockID());
+ Pipeline pipeline = allocatedBlock.getPipeline();
+ if (sortOnOm) {
Review Comment:
Use `sortedByNodes != null`. Then, we don't need the sortOnOm variable.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -2204,32 +2201,89 @@ 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) {
+ if (StringUtils.isEmpty(clientMachine)) {
+ // No client address: keep the pipeline order (the first node is the
write
+ // primary). Mirrors SCMBlockProtocolServer#getClientNode's empty guard.
+ return nodes;
+ }
Review Comment:
If clientMachine is empty, this method should not be called. So, let's add
a precondition:
```java
Preconditions.checkArgument(!StringUtils.isEmpty(clientMachine),
clientMachine);
```
--
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]
