[
https://issues.apache.org/jira/browse/HDFS-8210?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14548447#comment-14548447
]
Chris Nauroth commented on HDFS-8210:
-------------------------------------
Hi Jitendra. This is a great start. The separation of responsibilities
between {{StorageContainerMap}} and {{BitWiseTrieContainerMap}} makes sense.
Here are a few comments.
# Generally, we could use more comments, especially regarding how we manage the
keyspace and represent that in the trie. I defer to you on whether or not to
add comments right now or wait until a later date when the code is more
settled. We're on a feature branch, so we have flexibility.
# {{StorageContainerMap#iterator}}: Please add a more descriptive message to
the exception. Alternatively, does it make sense to go ahead and implement an
iterator that iterates through {{containerPrefixMap.values()}} and then
traverses the {{TrieNode}} instances for each one?
# {{StorageContainerMap#initPrefix}}: I believe there is a thread-safety
problem between calling {{getTrieMap}} (lock held) followed by
{{containerPrefixMap.put}} (lock not held). Does this whole method need to be
{{synchronized}}? Alternatively, maybe you could switch to a
{{ConcurrentHashMap}} and use {{putIfAbsent}}. Also, I wonder if the API would
be more consistent if the caller could pass in the prefix in the upper bits of
the argument (like {{getTrieMap}} and other methods expect) and let this class
encapsulate all of the bit manipulation. This patch doesn't yet call
{{initPrefix}} anywhere, so maybe passing it in the lower bits will make more
sense when I see that.
# {{StorageContainerMap#splitContainer}}: The call to
{{BitWiseTrieContainerMap#addBit}} is thread-safe, but I wonder if the
operation of splitting a container is going to need a stronger guarantee of
idempotence, so that multiple concurrent requests that need a split don't
result in multiple {{addBit}} calls. This might give rise to an API more like
{{ensureBits(int n)}}, with a post-condition that the bit length is at least n.
# {{BitWiseTrieContainerMap}}: The constructor hard-codes replication to 3. Do
you expect to revisit this? If so, would you mark a TODO?
# {{BitWiseTrieContainerMap#getInternal}}: I'm wary of recursive methods in
Java, since we don't get tail-call optimization. However, in this case, I
think it's fine, because we expect the maximum depth of the trie to be small:
64 - {{prefixLength}} = 36. Would you please add a comment about this?
# {{TestStorageContainerMap}}: If our prefix length is 28, then we expect to be
able to split 36 times, correct? If so, would you please add a test that
splits 36 times and then asserts that the pre-condition check fails on the 37th
split attempt?
> Ozone: Implement storage container manager
> -------------------------------------------
>
> Key: HDFS-8210
> URL: https://issues.apache.org/jira/browse/HDFS-8210
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Jitendra Nath Pandey
> Assignee: Jitendra Nath Pandey
> Attachments: HDFS-8210-HDFS-7240.1.patch, HDFS-8210-HDFS-7240.2.patch
>
>
> The storage container manager collects datanode heartbeats, manages
> replication and exposes API to lookup containers. This jira implements
> storage container manager by re-using the block manager implementation in
> namenode. This jira provides initial implementation that works with
> datanodes. The additional protocols will be added in subsequent jiras.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
