You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sumitagrawl (via GitHub)" <gi...@apache.org> on 2023/11/09 13:55:49 UTC
[PR] HDDS-9661. Datanode should not need to download existing container [ozone]
sumitagrawl opened a new pull request, #5576:
URL: https://github.com/apache/ozone/pull/5576
## What changes were proposed in this pull request?
check is added SendContainerRequestHandler which receives container data from source at target node, to reject if already import in progress or container is present at target.
This is do avoid resource usages and fail fast.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9661
## How was this patch tested?
- Unit case added
--
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
Re: [PR] HDDS-9661. Datanode should not need to download existing container [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel merged PR #5576:
URL: https://github.com/apache/ozone/pull/5576
--
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
Re: [PR] HDDS-9661. Datanode should not need to download existing container [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5576:
URL: https://github.com/apache/ozone/pull/5576#discussion_r1391941996
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SendContainerRequestHandler.java:
##########
@@ -80,6 +82,14 @@ public void onNext(SendContainerRequest req) {
LOG.info("Accepting container {}", req.getContainerID());
}
+ // check and avoid download of container file if target already have
+ // container data and import in progress
+ if (!importer.isAllowedContainerImport(containerId)) {
+ throw new StorageContainerException("Container exists or " +
+ "import in progress with container Id " + containerId,
+ ContainerProtos.Result.CONTAINER_EXISTS);
+ }
Review Comment:
@adoroszlai all comments updated
--
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
Re: [PR] HDDS-9661. Datanode should not need to download existing container [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5576:
URL: https://github.com/apache/ozone/pull/5576#discussion_r1391519528
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java:
##########
@@ -79,6 +79,16 @@ public ContainerImporter(ConfigurationSource conf, ContainerSet containerSet,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
}
+ public boolean isAllowedContainerImport(long containerID) {
+ if (importContainerProgress.contains(containerID)) {
+ return false;
+ }
+ if (containerSet.getContainer(containerID) != null) {
+ return false;
+ }
+ return true;
Review Comment:
Nit: can be simplified.
```suggestion
return !importContainerProgress.contains(containerID) &&
containerSet.getContainer(containerID) == null;
```
Also, please add `<>` in `new HashSet()`, missed in last change (1cf54f34af3).
https://github.com/apache/ozone/blob/4e95b702a996ec65f9be6f4d50a4025e55991d89/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java#L63-L64
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SendContainerRequestHandler.java:
##########
@@ -80,6 +82,14 @@ public void onNext(SendContainerRequest req) {
LOG.info("Accepting container {}", req.getContainerID());
}
+ // check and avoid download of container file if target already have
+ // container data and import in progress
+ if (!importer.isAllowedContainerImport(containerId)) {
+ throw new StorageContainerException("Container exists or " +
+ "import in progress with container Id " + containerId,
+ ContainerProtos.Result.CONTAINER_EXISTS);
+ }
Review Comment:
We can move this check before the `if` block (and change it to use `req.getContainerID()`) to avoid creating some resources unnecessarily.
--
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