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/04/25 02:59:19 UTC
[GitHub] [ozone] GlenGeng commented on a change in pull request #2172: HDDS-5126.Recon should check new containers of a container report wit…
GlenGeng commented on a change in pull request #2172:
URL: https://github.com/apache/ozone/pull/2172#discussion_r619739616
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
##########
@@ -122,26 +122,87 @@ public void checkAndAddNewContainer(ContainerID containerID,
scmClient.getContainerWithPipeline(containerID.getId());
LOG.debug("Verified new container from SCM {}, {} ",
containerID, containerWithPipeline.getPipeline().getId());
- // If no other client added this, go ahead and add this container.
- if (!containerExist(containerID)) {
- addNewContainer(containerID.getId(), containerWithPipeline);
- }
+ // no need call "containerExist" to check, because
+ // 1 containerExist and addNewContainer can not be atomic
+ // 2 addNewContainer will double check the existence
+ addNewContainer(containerWithPipeline);
} else {
- // Check if container state is not open. In SCM, container state
- // changes to CLOSING first, and then the close command is pushed down
- // to Datanodes. Recon 'learns' this from DN, and hence replica state
- // will move container state to 'CLOSING'.
- ContainerInfo containerInfo = getContainer(containerID);
- if (containerInfo.getState().equals(HddsProtos.LifeCycleState.OPEN)
- && !replicaState.equals(ContainerReplicaProto.State.OPEN)
- && isHealthy(replicaState)) {
- LOG.info("Container {} has state OPEN, but Replica has State {}.",
- containerID, replicaState);
- try {
- updateContainerState(containerID, FINALIZE);
- } catch (InvalidStateTransitionException e) {
- throw new IOException(e);
- }
+ checkContainerStateAndUpdate(containerID, replicaState);
+ }
+ }
+
+ /**
+ * Check and add new containers in batch if not already present in Recon.
+ *
+ * @param containerReplicaProtoList list of containerReplicaProtos.
+ * @throws IOException on Error.
+ */
+ public void checkAndAddNewContainerBatch(
+ List<ContainerReplicaProto> containerReplicaProtoList)
+ throws IOException {
+ Map<Boolean, List<ContainerReplicaProto>> containers =
+ containerReplicaProtoList.parallelStream()
+ .collect(Collectors.groupingBy(c ->
+ containerExist(ContainerID.valueOf(c.getContainerID()))));
+
+ List<ContainerReplicaProto> existContainers = null;
+ if (containers.containsKey(true)) {
+ existContainers = containers.get(true);
+ }
+ List<Long> noExistContainers = null;
+ if (containers.containsKey(false)){
+ noExistContainers = containers.get(false).parallelStream().
+ map(ContainerReplicaProto::getContainerID)
+ .collect(Collectors.toList());
+ }
+
+ //for now , if any one container in noExistContainers is not found by SCM,
+ //an IOException will be throw and the whole noExistContainers will be drop.
+ //in some cases，this may slow the process for recon to learn new container,
+ //but it does not matter, just make it simple for the present
+ if (null != noExistContainers) {
+ List<ContainerWithPipeline> verifiedContainerPipeline =
+ scmClient.getContainerWithPipelineBatch(noExistContainers);
Review comment:
The implementation of `getContainerWithPipelineBatch` in `SCMClientProtocolServer` is
```
StringBuilder strContainerIDs = new StringBuilder();
for (Long containerID : containerIDs) {
try {
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
strContainerIDs.append(ContainerID.valueOf(containerID).toString());
strContainerIDs.append(",");
} catch (IOException ex) {
AUDIT.logReadFailure(buildAuditMessageForFailure(
SCMAction.GET_CONTAINER_WITH_PIPELINE_BATCH,
Collections.singletonMap("containerID",
ContainerID.valueOf(containerID).toString()), ex));
throw ex;
}
}
```
Say the batch is `C1, C2, C3 C4`, `C2` is not existed in SCM, will C3 and C4 be ignored ?
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
##########
@@ -122,26 +122,87 @@ public void checkAndAddNewContainer(ContainerID containerID,
scmClient.getContainerWithPipeline(containerID.getId());
LOG.debug("Verified new container from SCM {}, {} ",
containerID, containerWithPipeline.getPipeline().getId());
- // If no other client added this, go ahead and add this container.
- if (!containerExist(containerID)) {
- addNewContainer(containerID.getId(), containerWithPipeline);
- }
+ // no need call "containerExist" to check, because
+ // 1 containerExist and addNewContainer can not be atomic
+ // 2 addNewContainer will double check the existence
+ addNewContainer(containerWithPipeline);
} else {
- // Check if container state is not open. In SCM, container state
- // changes to CLOSING first, and then the close command is pushed down
- // to Datanodes. Recon 'learns' this from DN, and hence replica state
- // will move container state to 'CLOSING'.
- ContainerInfo containerInfo = getContainer(containerID);
- if (containerInfo.getState().equals(HddsProtos.LifeCycleState.OPEN)
- && !replicaState.equals(ContainerReplicaProto.State.OPEN)
- && isHealthy(replicaState)) {
- LOG.info("Container {} has state OPEN, but Replica has State {}.",
- containerID, replicaState);
- try {
- updateContainerState(containerID, FINALIZE);
- } catch (InvalidStateTransitionException e) {
- throw new IOException(e);
- }
+ checkContainerStateAndUpdate(containerID, replicaState);
+ }
+ }
+
+ /**
+ * Check and add new containers in batch if not already present in Recon.
+ *
+ * @param containerReplicaProtoList list of containerReplicaProtos.
+ * @throws IOException on Error.
+ */
+ public void checkAndAddNewContainerBatch(
+ List<ContainerReplicaProto> containerReplicaProtoList)
+ throws IOException {
+ Map<Boolean, List<ContainerReplicaProto>> containers =
+ containerReplicaProtoList.parallelStream()
Review comment:
`stream().filter()` may be simpler and more straightforward here.
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
##########
@@ -122,26 +122,87 @@ public void checkAndAddNewContainer(ContainerID containerID,
scmClient.getContainerWithPipeline(containerID.getId());
LOG.debug("Verified new container from SCM {}, {} ",
containerID, containerWithPipeline.getPipeline().getId());
- // If no other client added this, go ahead and add this container.
- if (!containerExist(containerID)) {
- addNewContainer(containerID.getId(), containerWithPipeline);
- }
+ // no need call "containerExist" to check, because
+ // 1 containerExist and addNewContainer can not be atomic
+ // 2 addNewContainer will double check the existence
Review comment:
If we finally adopt the batch solution, can we drop this singular method ?
--
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