[
https://issues.apache.org/jira/browse/HDDS-123?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16497066#comment-16497066
]
Hanisha Koneru commented on HDDS-123:
-------------------------------------
Thanks for working on this [~bharatviswa].
A few comments:
* In {{ContainerSet#addContainer}}, if the containerID already exists in the
{{containerMap}}, we should through an exception. Or return false to the
calling function.
Also, can we add an info message after the container is added to the
containerMap and an error message otherwise.
* If {{ContainerSet#removeContainer}} is called twice on the same container,
the second call would throw NullPointerException due to the Precondition check.
{code:java}
Preconditions.checkNotNull(removed, "Container removed is null");{code}
We do not need to fail the operation if the container is already removed from
the map. We should instead log an info/error message on success/ failure
respectively.
* In {{ContaienrSet#listContainer}}, can we make the startContainerID
inclusive?
* In {{ContainerSet#getState}}, since it is private and is only used by
{{getContainerReport}}, we can pass the {{containerData}} directly instead of
looking it up again through containerID. We can avoid the null check also.
* A NIT: Can we have the debug logs in ContainerSet operations after the
operation is completed? Or change the message to indicate that the operation is
being performed.
{code}
LOG.debug("Container with container Id {} is added to containerMap",
containerId);
containerMap.put(containerId, container);
{code}
* Can we break down {{TestContainerSet#test}} into individual tests. For
example, separate tests for testing ContainerState, list container operation,
remove container operation etc.
> ContainerSet class to manage ContainerMap
> ------------------------------------------
>
> Key: HDDS-123
> URL: https://issues.apache.org/jira/browse/HDDS-123
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
> Reporter: Bharat Viswanadham
> Assignee: Bharat Viswanadham
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-123-HDDS-48.00.patch, HDDS-123-HDDS-48.01.patch,
> HDDS-123-HDDS-48.02.patch
>
>
> Create a ContainerSet class, which manages containerMap.
> Previously container map is in ContainerManagerImpl, with refactoring work it
> should be moved to ContainerSet.
> This class should handle add/get/remove container from containerMap.
> And also now it should handle containerReport.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
