[
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: [email protected]
For additional commands, e-mail: [email protected]
