[ https://issues.apache.org/jira/browse/HDDS-364?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16589114#comment-16589114 ]
Ajay Kumar edited comment on HDDS-364 at 8/22/18 4:56 PM: ---------------------------------------------------------- [~elek] thanks for review. Please see my response inline: {quote}What do you think about Node2ContainerMap? Do we need to update it as well?{quote} I think we can get rid of processReport by sending missing and new containers (deltas) directly form DN. This will reduce the SCM-DN communication and will make DN responsible for sending only changed state. But this can be done in separate jira. {quote}2. As I see all the containers will be persisted twice. (First, they will be imported, after that they will be reconciled.). Don't think it's a big problem. IMHO later we need to cleanup all the processing path anyway. One option may be just saving all the initial data to the state map without processing the reports (checking the required closing state, etc.). The downside here is some action would be delayed until the first real container report.{quote} Not sure which codepath you are referring here. This patch adds containers reported in register call to replicaMap which is in memory. Everything else remains same. {quote} The import part (in case of isRegisterCall=true) is the first part of processContainerReport method. I think it would be very easy to move to a separated method and call it independently from SCMDatanodeProtocolServer.register method. Could be more simple, and maybe it could be easier to test. Currently (as I understood) there is no specific test to test the isRegisterCall=true path. But this is not a blocking problem. Depends from your consideration{quote} Approach you are suggesting is close to the attached first patch. Had a discussion regarding this with [~xyao]. Moving it to processContainerReport is a small optimization to not iterate through whole list. (For a DN with 24 disks of 12 TB each we can have roughly 57600 containers of 5Gb) iterating through it and adding it to replicaMap should be quick but a large cluster with large no of DN may overwhelm the SCM during initial registration process. Moving this inside processContainerReport results in only one iteration of that list. At some point we can refactor this along with logic in ContainerCommandHandler and Node2ContainerMap#processReport. was (Author: ajayydv): [~elek] thanks for review. Please see my response inline: {quote}What do you think about Node2ContainerMap? Do we need to update it as well?{quote} I think we can get rid of processReport by sending missing and new containers (deltas) directly form DN. This will reduce the SCM-DN communication and will make DN responsible for sending only changed state. But this can be done in separate jira. {quote}2. As I see all the containers will be persisted twice. (First, they will be imported, after that they will be reconciled.). Don't think it's a big problem. IMHO later we need to cleanup all the processing path anyway. One option may be just saving all the initial data to the state map without processing the reports (checking the required closing state, etc.). The downside here is some action would be delayed until the first real container report.{quote} Not sure which codepath you are referring here. This patch adds containers reported in register call to replicaMap which is in memory. Everything else remains same. {quote} The import part (in case of isRegisterCall=true) is the first part of processContainerReport method. I think it would be very easy to move to a separated method and call it independently from SCMDatanodeProtocolServer.register method. Could be more simple, and maybe it could be easier to test. Currently (as I understood) there is no specific test to test the isRegisterCall=true path. But this is not a blocking problem. Depends from your consideration{quote} Approach you are suggesting is close to the attached first patch. Had a discussion regarding this with [~xyao]. Moving it to processContainerReport is a small optimization to not iterate through whole list. (For a DN with 24 disks of 12 TB each we can have roughly 57600 of 5Gb) iterating through it and adding it to replicaMap should be quick but a large cluster with large no of DN may overwhelm the SCM during initial registration process. Moving this inside processContainerReport results in only one iteration of that list. At some point we can refactor this along with logic in ContainerCommandHandler and Node2ContainerMap#processReport. > Update open container replica information in SCM during DN register > ------------------------------------------------------------------- > > Key: HDDS-364 > URL: https://issues.apache.org/jira/browse/HDDS-364 > Project: Hadoop Distributed Data Store > Issue Type: New Feature > Reporter: Ajay Kumar > Assignee: Ajay Kumar > Priority: Major > Fix For: 0.2.1 > > Attachments: HDDS-364.00.patch, HDDS-364.01.patch > > > Update open container replica information in SCM during DN register. -- 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