hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nanda kumar (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-11699) Ozone:SCM: Add support for close containers in SCM
Date Thu, 15 Feb 2018 16:35:00 GMT

    [ https://issues.apache.org/jira/browse/HDFS-11699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16365856#comment-16365856
] 

Nanda kumar commented on HDFS-11699:
------------------------------------

Thanks [~anu] for working on this. Please find my comments below


 *ContainerCloser.java*
Line:119 Typo "{{The container is close command is"}} -> "{{The container close command
is"}}
Line:138, 139, 142, 143 Multiple new/blank lines.
Line:146 method argument {{info}} is not required.
Line:155 {{MULTIPLIER * reportInterval}} can be done once inside the constructor and reused.
Line:171 Typo "{{setthe"}}

What is the need for launching a new instance of {{Closed Container Cleaner Thread}} if there
is already a cleaner thread running? can we change it such that only one instance of cleaner
thread is running at a time?

*ContainerMapping.java*
Naming suggestion: {{closed}} -> {{closeContainerIfFull}} or {{closeContainerIfNeeded.}}

Inside {{closed}} method we don't have to check both {{isClosed(scmInfo)}} and {{shouldClose(scmInfo)}},
one should be enough.

Line:478 We don't have to read {{ContainerInfo}} from db again, {{newState}} can be used instead.
{{newState}} has its container state updated from db already.

The below code
{code:java}
  private boolean closed(OzoneProtos.SCMContainerInfo newState)
      throws IOException {
    float containerUsedPercentage = 1.0f *
        newState.getUsedBytes() / this.size;

    ContainerInfo scmInfo = getContainer(newState.getContainerName());
    if (containerUsedPercentage >= containerCloseThreshold &&
        !isClosed(scmInfo)) {
      // We will call closer till get to the closed state.

      closer.close(newState);

      if (shouldClose(scmInfo)) {
        // This event moves the Container from Open to Closing State.
        OzoneProtos.LifeCycleState state = updateContainerState(
            scmInfo.getContainerName(),
            OzoneProtos.LifeCycleEvent.FINALIZE);
        if (state != OzoneProtos.LifeCycleState.CLOSING) {
          LOG.error("Failed to close container {}, reason : Not able " +
                  "to " +
                  "update container state, current container state: {}.",
              newState.getContainerName(), state);
          return false;
        }
        return true;
      }
    }
    return false;
  }
{code}
can be refactored to
{code:java}
  private boolean closed(OzoneProtos.SCMContainerInfo newState)
      throws IOException {
    float containerUsedPercentage = 1.0f *
        newState.getUsedBytes() / this.size;
    
    if (containerUsedPercentage >= containerCloseThreshold && shouldClose(newState))
{
      // We will call closer to get to the closed state.
      closer.close(newState);
      // This event moves the Container from Open to Closing State.
      OzoneProtos.LifeCycleState state = updateContainerState(
          newState.getContainerName(),
          OzoneProtos.LifeCycleEvent.FINALIZE);
      if (state == OzoneProtos.LifeCycleState.CLOSING) {
        return true;
      }
      LOG.error("Failed to close container {}, reason : Not able to " +
              "update container state, current container state: {}.",
          newState.getContainerName(), state);
    }
    return false;
  }
{code}
 

Should we handle {{IOException}} with try cache block in {{closed}} method? In {{closed}}
we get {{IOException}} while making {{updateContainerState}} call, we would like to continue
with the processing of next container even in case of IOException in {{closed}} method. Throwing
IOException will skip processing the rest of container reports.

Line:414, 415 Multiple new lines.
Line:452 {{datanodeState.getUsed()}} can be replaced with {{usedSize}}

Line:511 {{shouldClose}} method doesn't throw any IOException, {{throws}} from method signature
can be removed.

*SCMNodeManager.java*
This file only has a new line change; this can be reverted.

*TestContainerCloser.java*
Line:147 This line does nothing, can be removed.

> Ozone:SCM: Add support for close containers in SCM
> --------------------------------------------------
>
>                 Key: HDFS-11699
>                 URL: https://issues.apache.org/jira/browse/HDFS-11699
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Anu Engineer
>            Assignee: Anu Engineer
>            Priority: Major
>         Attachments: HDFS-11699-HDFS-7240.001.patch, HDFS-11699-HDFS-7240.002.patch
>
>
> Add support for closed containers in SCM. When a container is closed, SCM needs to make
a set of decisions like which pool and which machines are expected to have this container.
SCM also needs to issue a copyContainer command to the target datanodes so that these nodes
can replicate data from the original locations.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message