[
https://issues.apache.org/jira/browse/HDDS-173?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16523342#comment-16523342
]
Xiaoyu Yao commented on HDDS-173:
---------------------------------
Thanks [~hanishakoneru] for working on this and [~bharatviswa] for the review
discussions. The patch v2 looks good to me overall. I just have a few minor
comments.
ChunkUtils.java (keyvalue/helpers)
Line 326: we should checkNotNull for data and info as well to avoid NPE.
ContainerSet.java
Line 63: remove the throw StorageContainerException in the declaration.
ContainerUtils.java
Line 71-72/77: the comments don't seem to match with the code.
Line 90-92, 107-108: the comments need to be updated.
Line 128: NIT: unnecessary formatting change
Line 436: Should we return an Enum/Boolean instead of Exception here? Or add a
Container#isOpen/use Container#getContainerState()?
KeyUtils.java
Now we have two version, one in common/helpers and one in keyvalue/helpers. Is
it possible to consolidate to avoid duplicate code such as
getDB/RemoveDB/shutdownCache, etc.?
TestKeyManagerImpl.java
Line 172: NIT: we can skip the keyData by passing a new BlockID(1L,2L) here.
VolumeSet.java
Line 105: can we define Ozone/HDDS write lock configuration keys instead of
using the HDFS Namenode configuration keys?
Handler.java
Line 59:NIT: ideally, this should be a singleton handler for each container
type.
We are passing the whole containerSet to all handlers without checking its
supported container type. As a result, the Handler#handle() method should check
the container#getContainerType to see if can handler the command for specific
container type before further processing in subclass like
KeyValueHandler#handle().
Line 60: can we add a unit test (or sample code) that implements a
testContainer other than the default KeyValueContainer? This way, 3rd party can
easily implement different storage container types easily. This can be added
later with a follow up JIRA.
KeyValueContainer.java
Line 21: NIT: unused imports
Line 72: NIT: expand wild card imports.
Line 122: volumeSet.acquierLock() should be moved before line 120, try {…
Line 168: Do we have a follow up JIRA to handle out of space error and I/O
error in the volume layer?
TestKeyValueHandler.java
Line 26/34/36/39/44: NIT: unused imports.
> Refactor Dispatcher and implement Handler for new ContainerIO design
> --------------------------------------------------------------------
>
> Key: HDDS-173
> URL: https://issues.apache.org/jira/browse/HDDS-173
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
> Reporter: Hanisha Koneru
> Assignee: Hanisha Koneru
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-173-HDDS-48.001.patch, HDDS-173-HDDS-48.002.patch
>
>
> Dispatcher will pass the ContainerCommandRequests to the corresponding
> Handler based on the ContainerType. Each ContainerType will have its own
> Handler. The Handler class will process the message.
--
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
