You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/11/23 18:14:12 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
adoroszlai opened a new pull request #1610:
URL: https://github.com/apache/ozone/pull/1610
## What changes were proposed in this pull request?
`KeyManagerImpl#listStatus` and the `sortDatanodeInPipeline` helper method sort datanodes using individual RPC call for each key location info.
Improvements in this change:
1. Process only the "latest" location version instead of all versions. If I understand correctly, only the "latest" version is used for read.
2. Keep track of processed pipelines. If another key or location refers to the same pipeline, avoid repeating the query.
Possible improvement left for later: send a single `sortDatanodes` request for all datanodes in all relevant pipelines, then create the per-pipeline lists locally.
https://issues.apache.org/jira/browse/HDDS-4473
## How was this patch tested?
Added unit test for `sortDatanodes` and added check in existing test case for `listStatus`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-736644270
Thanks @sodonnel for the reviews.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
swagle commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-733817460
cc: @avijayanhwx for compat check though upgrades.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-735952352
Thanks @sodonnel for digging the details on pipeline refresh, and pointing out pipeline ID being random. In this case I think we should use the set of node IDs, instead of the pipeline ID, as cache key.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-735076745
Filed HDDS-4523 for further improvement suggested by @sodonnel.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#discussion_r533323808
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2322,21 +2322,22 @@ void sortDatanodes(String clientMachine, OmKeyInfo... keyInfos) {
}
for (OmKeyLocationInfo k : key.getLocationList()) {
Pipeline pipeline = k.getPipeline();
- PipelineID pipelineId = pipeline.getId();
- List<DatanodeDetails> sortedNodes = sortedPipelines.get(pipelineId);
+ List<DatanodeDetails> nodes = pipeline.getNodes();
+ List<String> uuidList = toNodeUuid(nodes);
+ Set<String> uuidSet = new HashSet<>(uuidList);
Review comment:
I tried that first. The problem is we'd need to return a sorted set (for setting up reproducible expectations in unit tests), which somewhat goes against the goal of optimization.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-735943927
This change looks good to me and I am +1 to commit it. However I would like to highlight one issue that may make this change less effective.
In SCMClientProtocolServer this is the code where the refreshPipelinesBatch call lands.
```
private ContainerWithPipeline getContainerWithPipelineCommon(
long containerID) throws IOException {
final ContainerID cid = ContainerID.valueof(containerID);
final ContainerInfo container = scm.getContainerManager()
.getContainer(cid);
if (safeModePrecheck.isInSafeMode()) {
if (container.isOpen()) {
if (!hasRequiredReplicas(container)) {
throw new SCMException("Open container " + containerID + " doesn't"
+ " have enough replicas to service this operation in "
+ "Safe mode.", ResultCodes.SAFE_MODE_EXCEPTION);
}
}
}
Pipeline pipeline;
try {
pipeline = container.isOpen() ? scm.getPipelineManager()
.getPipeline(container.getPipelineID()) : null;
} catch (PipelineNotFoundException ex) {
// The pipeline is destroyed.
pipeline = null;
}
if (pipeline == null) {
pipeline = scm.getPipelineManager().createPipeline(
HddsProtos.ReplicationType.STAND_ALONE,
container.getReplicationFactor(),
scm.getContainerManager()
.getContainerReplicas(cid).stream()
.map(ContainerReplica::getDatanodeDetails)
.collect(Collectors.toList()));
}
return new ContainerWithPipeline(container, pipeline);
}
```
For open containers the current pipeline is returned, but for a closed container, a new "pipeline" is created. This is not a real ratis pipeline - its just a list of nodes. If you follow the logic to SimplePipelineProvider, the pipeline ID is a random ID:
```
public Pipeline create(ReplicationFactor factor,
List<DatanodeDetails> nodes) {
return Pipeline.newBuilder()
.setId(PipelineID.randomId())
.setState(PipelineState.OPEN)
.setType(ReplicationType.STAND_ALONE)
.setFactor(factor)
.setNodes(nodes)
.build();
}
```
This means, that even if several blocks have the same set of DNs, they will still be distinct pipelines, and hence the caching will not work. The caching will only be effective for open containers.
There are two places this logic is used - listStatus and lookupkey / getOzoneFileStatus.
I guess the place where this could be a win is in listStatus, but if most keys are part of closed containers (which would be the usual case) the sorted pipeline cache introduced here may cause more overhead than it saves.
I also wonder - why does ListStatus need to return the pipeline and optionally sort the pipeline for all keys, all the time?
I guess we can have two uses - simply listing the directory and displaying the contents - usually locations is not needed here. Second, an application list the folders with the intention of reading each key - in this second case the locations are needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #1610:
URL: https://github.com/apache/ozone/pull/1610
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#discussion_r533310687
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2322,21 +2322,22 @@ void sortDatanodes(String clientMachine, OmKeyInfo... keyInfos) {
}
for (OmKeyLocationInfo k : key.getLocationList()) {
Pipeline pipeline = k.getPipeline();
- PipelineID pipelineId = pipeline.getId();
- List<DatanodeDetails> sortedNodes = sortedPipelines.get(pipelineId);
+ List<DatanodeDetails> nodes = pipeline.getNodes();
+ List<String> uuidList = toNodeUuid(nodes);
+ Set<String> uuidSet = new HashSet<>(uuidList);
Review comment:
Do you think it would be better to return a Set<String> directly in toNodeUuid(...), rather than return a list and then create a set from it? Its a tiny optimisation but I don't think we really need the intermediate list - its just used in the log message later and we can probably just use "nodes" rather than nodeList in the log as it was before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-736393901
> we should use the set of node IDs, instead of the pipeline ID, as cache key.
Good idea - I think that will work much better. I checked the latest version and it looks good to me. I just have one small comment you can consider if you think it makes sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-733111102
I noticed this code before, in that we have a batch call to get the pipelines, but then for a key with multiple blocks, we must make a call per block to sort the datanodes. Am I correct in thinking this is mostly a problem with large keys which have many blocks?
Do you think it would make sense to add the sort flag to the `getContainerWithPipeline` and `getContainerWithPipelineBatch` so we can have the pipeline returned sorted with a single RPC call, rather than having to make at least two calls?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#discussion_r533333586
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2322,21 +2322,22 @@ void sortDatanodes(String clientMachine, OmKeyInfo... keyInfos) {
}
for (OmKeyLocationInfo k : key.getLocationList()) {
Pipeline pipeline = k.getPipeline();
- PipelineID pipelineId = pipeline.getId();
- List<DatanodeDetails> sortedNodes = sortedPipelines.get(pipelineId);
+ List<DatanodeDetails> nodes = pipeline.getNodes();
+ List<String> uuidList = toNodeUuid(nodes);
+ Set<String> uuidSet = new HashSet<>(uuidList);
Review comment:
OK - thanks for the explanation. We can keep it how it is then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #1610: HDDS-4473. Reduce number of sortDatanodes RPC calls
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1610:
URL: https://github.com/apache/ozone/pull/1610#issuecomment-733127142
> Am I correct in thinking this is mostly a problem with large keys which have many blocks?
I think so, and also for `listStatus` with many entries in the result.
> Do you think it would make sense to add the sort flag to the `getContainerWithPipeline` and `getContainerWithPipelineBatch` so we can have the pipeline returned sorted with a single RPC call, rather than having to make at least two calls?
Actually that's a great idea, better than the third (and not yet implemented) improvement I proposed, which would still require 2 batch requests.
I think we have to pay attention to compatibility, though. Client would need to check if SCM supports this new flag, so that it can get sorted pipeline from both new and old SCM.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org