[
https://issues.apache.org/jira/browse/HDDS-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16525589#comment-16525589
]
Hanisha Koneru edited comment on HDDS-183 at 6/27/18 8:40 PM:
--------------------------------------------------------------
Thanks for working on this [~bharatviswa].
A few comments:
* ContainerReader
** Line 106, 112 : if check redundant
** Line 160, 164 : Catch blocks can be combined as one.
** Line 147 : We should not cast to KeyValueContainerData
** verifyContainerFile() - If containerType does not match KeyValueContainer,
we should log an error.
** parseKeyValueContainerData() can have void return type as the containerData
is updated in place.
Also, can we move this function to KeyValueContainerUtils or some other
KeyValue class. It would be good to keep the common implementations free of
specific containerType code.
** ContainerFilter class is unused.
* ContainerDataYaml
** Line 84, 174 - new Tag("KeyValueContainerData”) should be defined as a
static variable in KeyValueContainerData or KeyValueContainer class.
** Line 207 - state can be “INVALID” also.
** Line 152-157 : Instead of defining the valid fields here, can we maintain a
list of valid yaml fields for KVContainer in KVContainerData. This way, if a
new field is added to KVContainerData, we can update this list itself and avoid
"When a new field needs to be added, it needs to be added here.” Also, these
fields should be defined as static final varaibles.
** It would be good to move ConstructKeyValueContainerData class also to
keyvalue package.
* DeleteBlocksCommandHandler - we should handle different containerTypes here.
For types other than KV, we can just log an error.
* VersionEndpointTask
** Line 70 : keyValues not used.
** Line 84 : ozoneContainer.getDispatcher().setScmId(scmId); should be outside
the for loop for volumeMap.
* HddsVolume - changes are not required.
* VolumeSet - NIT - Line 358 : Unused variable srb
* KeyValueContainerUtil - NIT - Line 213 : check sum -> checksum, Container
Name -> Container ID (just to avoid confusion for admins when debugging logs).
* KeyValueHandler
** We need the kvContainer.writeUnlock() in handleDeleteContainer(). When we
detect that the container is open, we should immediately release the lock and
not wait for any other operation (even loggging).
* SCMTestUtils - can we replace DFS_DATANODE_DATA_DIR_KEY with
HDDS_DATANODE_DIR_KEY.
was (Author: hanishakoneru):
Thanks for working on this [~bharatviswa].
A few comments:
* ContainerReader
** Line 106, 112 : if check redundant
** Line 160, 164 : Catch blocks can be combined as one.
** Line 147 : We should not cast to KeyValueContainerData
** verifyContainerFile() - If containerType does not match KeyValueContainer,
we should log an error.
** parseKeyValueContainerData() can have void return type as the containerData
is updated in place.
Also, can we move this function to KeyValueContainerUtils or some other
KeyValue class. It would be good to keep the common implementations free of
specific containerType code.
** ContainerFilter class is unused.
* ContainerDataYaml
** Line 84, 174 - new Tag("KeyValueContainerData”) should be defined as a
static variable in KeyValueContainerData or KeyValueContainer class.
** Line 207 - state can be “INVALID” also.
** Line 152-157 : Instead of defining the valid fields here, can we maintain a
list of valid yaml fields for KVContainer in KVContainerData. This way, if a
new field is added to KVContainerData, we can update this list itself and avoid
"When a new field needs to be added, it needs to be added here.” Also, these
fields should be defined as static final varaibles.
** It would be good to move ConstructKeyValueContainerData class also to
keyvalue package.
* DeleteBlocksCommandHandler - we should handle different containerTypes here.
For types other than KV, we can just log an error.
* VersionEndpointTask
** Line 70 : keyValues not used.
** Line 84 : ozoneContainer.getDispatcher().setScmId(scmId); should be outside
the for loop for volumeMap.
* HddsVolume - changes are not required.
* VolumeSet - NIT - Line 358 : Unused variable srb
* KeyValueContainerUtil - NIT - Line 213 : check sum -> checksum, Container
Name -> Container ID (just to avoid confusion for admins when debugging logs).
* KeyValueHandler
** We need the kvContainer.writeUnlock() in handleDeleteContainer(). When we
detect that the container is open, we should immediately release the lock and
not wait for any other operation (even loggging).
> Integrate Volumeset, ContainerSet and HddsDispatcher
> ----------------------------------------------------
>
> Key: HDDS-183
> URL: https://issues.apache.org/jira/browse/HDDS-183
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
> Reporter: Bharat Viswanadham
> Assignee: Bharat Viswanadham
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-183-HDDS-48.00.patch, HDDS-183-HDDS-48.01.patch,
> HDDS-183-HDDS-48.02.patch, HDDS-183-HDDS-48.03.patch
>
>
> This Jira adds following:
> 1. Use new VolumeSet.
> 2. build container map from .container files during startup.
> 3. Integrate HddsDispatcher.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
