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 2021/12/09 14:53:06 UTC
[GitHub] [ozone] sodonnel opened a new pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
sodonnel opened a new pull request #2903:
URL: https://github.com/apache/ozone/pull/2903
## What changes were proposed in this pull request?
The logic in `SimpleContainerDownloader.getContainerDataFromReplicas()` is hard to follow and could be written in a simpler way, with the same functionality.
This Jira is a small refactor of the logic in the above method, which hopefully makes it easier to understand.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6080
## How was this patch tested?
No functionality changes. Existing tests cover this.
--
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: issues-unsubscribe@ozone.apache.org
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 merged pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
sodonnel merged pull request #2903:
URL: https://github.com/apache/ozone/pull/2903
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was trying to call `downloadContainer()` one more time before giving up?
This "retry" logic was added in:
https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179#diff-57c91f8c82e693f5b3f1b71b8d1bb5d719c1e86eb82208b1cf9616c740654d9dR86-R96
not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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 #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766545933
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
- }
+ CompletableFuture<Path> result =
+ downloadContainer(containerId, datanode);
+ result.get();
Review comment:
> It looks like the only caller of this method DownloadAndImportReplicator#replicate is also handling the same set of exceptions when calling CompletableFuture#get()
Yea I noticed that - perhaps I will change it to just return the path or null, and then we can remove the exception handling from the caller too.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was just trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was initially added in https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179#diff-57c91f8c82e693f5b3f1b71b8d1bb5d719c1e86eb82208b1cf9616c740654d9dR86-R96
, not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was just trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was initially added in https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179#diff-57c91f8c82e693f5b3f1b71b8d1bb5d719c1e86eb82208b1cf9616c740654d9dR86-R96 , not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was just trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was initially added in:
https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179#diff-57c91f8c82e693f5b3f1b71b8d1bb5d719c1e86eb82208b1cf9616c740654d9dR86-R96
, not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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 #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766548709
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
- }
+ CompletableFuture<Path> result =
+ downloadContainer(containerId, datanode);
+ result.get();
+ return result;
+ } catch (ExecutionException e) {
Review comment:
The `catch Exception` at the bottom should handle IOExceptions too, but I can add this one back in:
```
} catch (Exception ex) {
LOG.error("Container {} download from datanode {} was unsuccessful. "
+ "Trying the next datanode", containerId, datanode, ex);
```
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was just trying to run `downloadContainer()` one more time before printing error?
--
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: issues-unsubscribe@ozone.apache.org
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 #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766200831
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
I'm not sure. I looked at the original code for a long time to try to understand it, which is the problem I am trying to solve. I don't think its intention / logic is clear. This is the original code:
```
@Override
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
CompletableFuture<Path> result = null;
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
if (result == null) {
result = downloadContainer(containerId, datanode);
} else {
result = result.exceptionally(t -> {
LOG.error("Error on replicating container: " + containerId, t);
try {
return downloadContainer(containerId, datanode).get();
} catch (ExecutionException | IOException e) {
LOG.error("Error on replicating container: " + containerId,
e);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
return null;
});
}
} catch (Exception ex) {
LOG.error(String.format(
"Container %s download from datanode %s was unsuccessful. "
+ "Trying the next datanode", containerId, datanode), ex);
}
}
return result;
```
My reading of it:
* On the first pass of the loop, result will be null so the download is attempted via the future. However we don't check for completion, so the loop must go into a second pass.
* On the second pass (assuming there is more than 1 DN). Result will be non-null, so it will call `result.exceptionally()`. Reading the java doc for it, which is confusing to say the least, I think it will "complete" the future if it was successful, or run the lambda if not. At this stage, the lambda is using the second DN in the list rather than the first, so the retry will be on the second DN, not a second try on the first.
* If there is a 3rd DN in the list, I think it will still loop again, as there is no break in the loop and no return (aside from the one in the lambda), but this time result is non-null and completed if the second pass worked, so I am guessing calling exceptionally on it does nothing, but I am not certain.
* Then the loop exits and the method returns.
If there is only a single DN in the list, then I think it doesn't actually ever call exceptionally, so the future doesn't get completed until the method returns in `DownloadAndImportReplicator.replicate()` where we call "get" on the future.
I think the new code performs the same number of retries, but I hope, in an easier to follow way.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was added in:
https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179
, not sure if it still makes sense. If not, I am +1.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was added in:
, not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was trying to call `downloadContainer()` one more time before giving up?
The "retry" logic was added in:
https://github.com/apache/ozone/commit/facf36edb49660652b08aeaaec36d3467e0fd179#diff-57c91f8c82e693f5b3f1b71b8d1bb5d719c1e86eb82208b1cf9616c740654d9dR86-R96
not sure if it still makes sense. If not, I am +1.
--
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: issues-unsubscribe@ozone.apache.org
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 #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766544819
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
- }
+ CompletableFuture<Path> result =
+ downloadContainer(containerId, datanode);
+ result.get();
Review comment:
I am pretty certain that calling `get()` on a future simply waits for the future to complete and returns its result (which is the path in this case). Calling `get()` while the download is still in progress just blocks until it completes or throws an exception. Calling `get()` again, I suspect just returns the result (path) again, but I will check to be sure.
--
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: issues-unsubscribe@ozone.apache.org
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] smengcl commented on a change in pull request #2903: HDDS-6080. Simplify the logic in SimpleContainerDownloader.getContainerDataFromReplicas
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #2903:
URL: https://github.com/apache/ozone/pull/2903#discussion_r766170512
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SimpleContainerDownloader.java
##########
@@ -74,38 +74,28 @@ public SimpleContainerDownloader(
public CompletableFuture<Path> getContainerDataFromReplicas(
long containerId, List<DatanodeDetails> sourceDatanodes) {
- CompletableFuture<Path> result = null;
-
final List<DatanodeDetails> shuffledDatanodes =
shuffleDatanodes(sourceDatanodes);
for (DatanodeDetails datanode : shuffledDatanodes) {
try {
- if (result == null) {
- result = downloadContainer(containerId, datanode);
- } else {
-
- result = result.exceptionally(t -> {
- LOG.error("Error on replicating container: " + containerId, t);
- try {
- return downloadContainer(containerId, datanode).get();
- } catch (ExecutionException | IOException e) {
- LOG.error("Error on replicating container: " + containerId,
- e);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- return null;
- });
Review comment:
It seems to me that the original logic was just trying to call `downloadContainer()` one more time before giving up?
--
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: issues-unsubscribe@ozone.apache.org
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