[
https://issues.apache.org/jira/browse/HDFS-12665?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16214114#comment-16214114
]
Ewan Higgs commented on HDFS-12665:
-----------------------------------
[~virajith], thanks for the quick review.
{quote}Can you please add javadocs for all the new classes added?{quote}
Sure.
{quote}Is there a reason to refactor FileRegion and introduce
ProvidedStorageLocation? Also, I think the name ProvidedStorageLocation is
confusing given there is also a StorageLocation, which is something very
different. May be rename to ProvidedLocation.{quote}
The AliasMap is a mapping between a block and a location in an external storage
system so we need to break the FileRegion in two: the key and the value. The
Block is the key in this mapping and {{FileRegion}} is a good name for the
value being stored but it's taken by the entire KV entry itself.
{quote}The new AliasMap class has a confusing name. It is supposed to be an
implementation of the AliasMapProtocol but the name is a prefix of the
latter.{quote}
{{AliasMapProtocol}} is an interface (hence {{Protocol}} so the concrete
version doesn't have the suffix. It should be more clear when there are javadoc
comments attached to it.
{quote}Renaming LevelDBAliasMapClient to something along the lines
InMemoryLevelDBAliasMap will make it a more descriptive name for the class. In
general, adding a similar prefix to AliasMapProtocol, LevelDBAliasMapServer
will improve the readability of the code.{quote}Agree. This will also help
differentiate between the {{LevelDBFileRegionFormat}} (HDFS-12591).
{quote}Can we move LevelDBAliasMapClient to the
org.apache.hadoop.hdfs.server.common.BlockAliasMapImpl package. {quote}
Sure. This means all the classes used by fs2img will be in the same package
(unless they need dependencies like using DynamoDB, AzureTable, etc).
{quote}ITAliasMap only contains unit tests. I believe the convention is to
start the name of the class with Test.{quote}I think this was part of how the
code evolved. e.g. code using MiniDFSCluster was here but moved back to the
unit tests since the HDFS project has a different sense of what differentiates
unit and integration tests.
{quote}Why was the block pool id removed from FileRegion? It was used as a
check in the DN so that only blocks belonging to the correct block pool id were
reported to the NN.{quote}In an early version we refactored it to use
{{ExtendedBlock}} as the key but were advised that it should remain {{Block}}.
AIUI, the AliasMap is unique to a NN so there is no ambiguity.
{quote}Why rename getVolumeMap to fetchVolumeMap in
ProvidedBlockPoolSlice?{quote} The return type is {{void}} so naming this
{{getVolumeMap}} is misleading.
{quote}In startAliasMapServerIfNecessary, I think the aliasmap should be
started only if provided is configured. i.e., check if
DFSConfigKeys.DFS_NAMENODE_PROVIDED_ENABLED is set to true.{quote}That makes
sense. If an administrator is running with
{{DFSConfigKeys.DFS_NAMENODE_PROVIDED_ENABLED}} set to false but
{{DFSConfigKeys.DFS_USE_ALIASMAP}} set to true, it's pretty lame. Should we
throw or just log a warning about the misconfiguration.
{quote}Some of the changes have lead to lines crossing the 80 character limit.
Can you please fix them?{quote}Sure. It seems to be convention to ignore that
in {{DFSConfigKeys,java}} but I'll take a look at fixing this up elsewhere.
> [AliasMap] Create a version of the AliasMap that runs in memory in the
> Namenode (leveldb)
> -----------------------------------------------------------------------------------------
>
> Key: HDFS-12665
> URL: https://issues.apache.org/jira/browse/HDFS-12665
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Ewan Higgs
> Assignee: Ewan Higgs
> Attachments: HDFS-12665-HDFS-9806.001.patch,
> HDFS-12665-HDFS-9806.002.patch
>
>
> The design of Provided Storage requires the use of an AliasMap to manage the
> mapping between blocks of files on the local HDFS and ranges of files on a
> remote storage system. To reduce load from the Namenode, this can be done
> using a pluggable external service (e.g. AzureTable, Cassandra, Ratis).
> However, to aide adoption and ease of deployment, we propose an in memory
> version.
> This AliasMap will be a wrapper around LevelDB (already a dependency from the
> Timeline Service) and use protobuf for the key (blockpool, blockid, and
> genstamp) and the value (url, offset, length, nonce). The in memory service
> will also have a configurable port on which it will listen for updates from
> Storage Policy Satisfier (SPS) Coordinating Datanodes (C-DN).
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
