[
https://issues.apache.org/jira/browse/HDDS-175?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16523539#comment-16523539
]
Nanda kumar commented on HDDS-175:
----------------------------------
Thanks [~ajayydv] for working on such a huge refactoring change. Please find my
comments below.
*ContainerInfo*
Instead of {{expectedReplicaCount}} as a int value, we can use
{{ReplicationFactor}} enum from HddsProtos.
* {{fromProtobuf}}
{code:java}
List<DatanodeDetails> dnDetails = new ArrayList<>();
for(DatanodeDetailsProto dn: info.getDataNodesList()) {
DatanodeDetails dnDetail =
DatanodeDetails.newBuilder().setHostName(dn.getHostName())
.setIpAddress(dn.getIpAddress())
.setUuid(dn.getUuid()).build();
for (HddsProtos.Port port : dn.getPortsList()) {
dnDetail.setPort(DatanodeDetails.newPort(
Port.Name.valueOf(port.getName().toUpperCase()), port.getValue()));
}
dnDetails.add(dnDetail);
}
{code}
can be replaced with
{code:java}
List<DatanodeDetails> dnDetails = new ArrayList<>();
info.getDataNodesList().forEach(dd ->
dnDetails.add(DatanodeDetails.getFromProtoBuf(dd)));
{code}
* getDataNodesIds
{code:java}
public List<String> getDataNodesIds() {
if (dataNodes == null) {
return ListUtils.EMPTY_LIST;
}
List<String> nodes = Lists.newArrayListWithExpectedSize(dataNodes.size());
for (DatanodeDetails dn : dataNodes) {
nodes.add(dn.getUuidString());
}
return nodes;
}
{code}
can be refactored to
{code:java}
public List<String> getDataNodesIds() {
List<String> result = new ArrayList<>();
dataNodes.forEach(dn -> result.add(dn.getUuidString()));
return result;
}
{code}
* getProtobuf
In few {{Builder.setXXX}} calls we have used instance variable and in few we
have used getter methods. It will look good if we stick with any one, I'm
inclined towards using instance variable.
{code:java}
public HddsProtos.SCMContainerInfo getProtobuf() {
HddsProtos.SCMContainerInfo.Builder builder =
HddsProtos.SCMContainerInfo.newBuilder();
builder.setAllocatedBytes(getAllocatedBytes())
.setUsedBytes(getUsedBytes())
.setNumberOfKeys(getNumberOfKeys()).setState(state)
.setStateEnterTime(stateEnterTime).setContainerID(getContainerID())
.setDeleteTransactionId(deleteTransactionId)
.setPipelineId(getPipelineId())
.setExpectedCount(getExpectedReplicaCount())
.setReplicationType(getReplicationType());
int i = 0;
getDataNodes().forEach(dn -> {
DatanodeDetailsProto.Builder dnBuilder = DatanodeDetailsProto
.newBuilder()
.setHostName(dn.getHostName())
.setIpAddress(dn.getIpAddress())
.setUuid(dn.getUuidString());
for (Port port : dn.getPorts()) {
dnBuilder.addPorts(HddsProtos.Port.newBuilder()
.setName(port.getName().toString())
.setValue(port.getValue())
.build());
}
builder.addDataNodes(dnBuilder.build());
});
if (getOwner() != null) {
builder.setOwner(getOwner());
}
return builder.build();
}
{code}
can be refactored to
{code:java}
public HddsProtos.SCMContainerInfo getProtobuf() {
HddsProtos.SCMContainerInfo.Builder builder =
HddsProtos.SCMContainerInfo.newBuilder()
.setContainerID(containerID)
.setPipelineId(pipelineId)
.setAllocatedBytes(allocatedBytes)
.setUsedBytes(usedBytes)
.setNumberOfKeys(numberOfKeys)
.setState(state)
.setStateEnterTime(stateEnterTime)
.setDeleteTransactionId(deleteTransactionId)
.setExpectedCount(expectedReplicaCount)
.setReplicationType(replicationType)
.setOwner(owner);
dataNodes.forEach(dn -> builder.addDataNodes(dn.getProtoBufMessage()));
return builder.build();
}
{code}
* {{equals}} method implementation only uses {{containerId}} and {{owner}}.
{{hashCode}} should also use the same set of fields used by {{equals}}.
hashCode of equal objects should always be same.
*ContainerMapping*
{{allocateContainer}} should return {{ContaineInfo}} not
{{ContainerWithPipeline}}
*ClientPbHelper* : Not used anywhere, this class can be removed.
*OzonePBHelper* : Not used anywhere, this class can be removed.
Checkstyle issues (Not sure why this is not captured by jenkins run):
* ContainerInfo: *
Line:463 'dataNodes' hides a field
Line:469 'replicationType' hides a field
Line:474 'pipelineId' hides a field
Line:479 'expectedReplicaCount' hides a field
* BlockManagerImpl *
Line:31 Unused import {{org.apache.hadoop.hdds.scm.pipelines.PipelineSelector}}
> Refactor ContainerInfo to remove Pipeline object from it
> ---------------------------------------------------------
>
> Key: HDDS-175
> URL: https://issues.apache.org/jira/browse/HDDS-175
> Project: Hadoop Distributed Data Store
> Issue Type: Bug
> Components: SCM
> Affects Versions: 0.2.1
> Reporter: Ajay Kumar
> Assignee: Ajay Kumar
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-175.00.patch, HDDS-175.01.patch, HDDS-175.02.patch,
> HDDS-175.03.patch, HDDS-175.04.patch, HDDS-175.05.patch, HDDS-175.06.patch
>
>
> Refactor ContainerInfo to remove Pipeline object from it. We can add below 4
> fields to ContainerInfo to recreate pipeline if required:
> # pipelineId
> # replication type
> # expected replication count
> # DataNode where its replica exist
--
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
