[ https://issues.apache.org/jira/browse/HDDS-245?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16560153#comment-16560153 ]
Anu Engineer commented on HDDS-245: ----------------------------------- The changes look excellent. I am a +1 on this patch. Some small comments, most of them are non-actionable in this patch. As soon you take a look, we can probably commit this patch. * *ContainerReportHandler.java#ContainerReportHandler* Add a Precondition.NotNull check for \{containerMapping, node2ContainerMap, replicationActivityStatus} * *ContainerReportHandler.java#onMessage* {code:java} //update state in container db and trigger close container events containerMapping.processContainerReports(datanodeOrigin, containerReport); {code} I don't think this is what we should do. This function follows the old state logic. IMHO, we should replace this completely. However, we can commit this patch and go rewrite {{containerMapping.processContainerReports}} function later. The current implementation makes it the code kind of kludge. cc: [~nandakumar131]. * *ContainerReportHandler.java#onMessage* {code:java} Set<ContainerID> containerIds = containerReport.getReportsList().stream() .map(containerProto -> containerProto.getContainerID()) .map(ContainerID::new) .collect(Collectors.toSet()); {code} A nit: Technically a container report should *only* contain unique container IDs. But I am not sure if that is enforced on the datanode side. This change can loose some IDs. But no action is needed here. Just pointing out something that I thought about. * ContainerReportHandler.java#onMessage I am being generous today with my comments :). So I am going to share some history behind this code fragment. {code:java} ReportResult reportResult = node2ContainerMap .processReport(datanodeOrigin.getUuid(), containerIds); //we have the report, so we can update the states for the next iteration. node2ContainerMap .updateDatanodeMap(datanodeOrigin.getUuid(), containerIds); {code} My original proposal (exactly as the code in this patch) was that we will update the node report and return the resultset. [~nandakumar131] while reviewing the original design pointed out a strange problem, he said it is possible that you might update the Node2Container State, but fail to update the state in ContainerStateManager. If that happens, we will never detect the inconsistency because the Node2Container mapping is used to evaluate if we need to update the state of ContainerManager. So he proposed that we first compute the difference, then update ContainerStateManager and then come back and update the Node2Container map. Bottom line: He proposed that we do: # processReport # The Loop you have for processing the Missing Containers and new containers # The update the updateDatanodeMap – So the change is trivial and it is just defensive coding. The actual probability of something like this happening is very low. So move line 94 to Line 107 is the change. * A future feature request: If we get an IOException during the report processing, can we have a metric for that ? not only log it, but a metric that we can be used to monitor and flag this issue. * Node2ContainerMapping.java – Not sure why we are allocating new HashSets; isn't the calling function giving us a set already do we need to make a copy ? (Just scrolled up and saw [~xyao] comments) I am pretty sure they cannot be null, but it is not a big deal. * ReplicationActivityStatus: Great idea, and thanks for supporting this in this patch. > Handle ContainerReports in the SCM > ---------------------------------- > > Key: HDDS-245 > URL: https://issues.apache.org/jira/browse/HDDS-245 > Project: Hadoop Distributed Data Store > Issue Type: Improvement > Components: SCM > Reporter: Elek, Marton > Assignee: Elek, Marton > Priority: Major > Fix For: 0.2.1 > > Attachments: HDDS-245.001.patch, HDDS-245.002.patch, > HDDS-245.003.patch, HDDS-245.004.patch > > > HDDS-242 provides a new class ContainerReportHandler which could handle the > ContainerReports from the SCMHeartbeatDispatchere. > HDDS-228 introduces a new map to store the container -> datanode[] mapping > HDDS-199 implements the ReplicationManager which could send commands to the > datanodes to copy the datanode. > To wire all these components, we need to put implementation to the > ContainerReportHandler (created in HDDS-242). > The ContainerReportHandler should process the new ContainerReportForDatanode > events, update the containerStateMap and node2ContainerMap and calculate the > missing/duplicate containers and send the ReplicateCommand to the > ReplicateManager. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org