hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-3077) Quorum-based protocol for reading and writing edit logs
Date Fri, 06 Jul 2012 03:15:35 GMT

    [ https://issues.apache.org/jira/browse/HDFS-3077?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13407671#comment-13407671
] 

Aaron T. Myers commented on HDFS-3077:
--------------------------------------

I just finished a review of the latest patch. Overall it looks really good. Great test coverage,
too.

Some comments:

# If the following is supposed to be a list of host:port pairs, I suggest we call it something
other than "*.edits.dir". Also, if the default is just a path, is it really supposed to be
a list of host:port pairs? Or is this comment supposed to be referring to DFS_JOURNALNODE_RPC_ADDRESS_KEY?
{code}
+  // This is a comma separated host:port list of addresses hosting the journal service
+  public static final String  DFS_JOURNALNODE_EDITS_DIR_KEY = "dfs.journalnode.edits.dir";
+  public static final String  DFS_JOURNALNODE_EDITS_DIR_DEFAULT = "/tmp/hadoop/dfs/journalnode/";
{code}
# Could use a class comment and method comments in AsyncLogger.
# Missing an @param comment for AsyncLoggerSet#createNewUniqueEpoch.
# I think this won't substitute in the correct hostname in a multi-node setup with host-based
principal names:
{code}
+        SecurityUtil.getServerPrincipal(conf
+            .get(DFSConfigKeys.DFS_JOURNALNODE_USER_NAME_KEY),
+            NameNode.getAddress(conf).getHostName()) };
{code}
# In IPCLoggerChannel, I wonder if you also shouldn't ensure that httpPort is not yet set
here:
{code}
// Fill in HTTP port. TODO: is there a more elegant place to put this?
 httpPort = ret.getHttpPort();
{code}
# Is there no need for IPCLoggerChannel to have a way of closing its associated proxy?
# Could use some comments in JNStorage.
# Seems a little odd that JNStorage relies on a few static functions of NNStorage. Is there
some better place those functions could live?
# I don't understand why JNStorage#analyzeStorage locks the storage directory after formatting
it. What, if anything, relies on that behavior? Where is it unlocked? Might want to add a
comment explaining it.
# Patch needs to be rebased on trunk, e.g. PersistentLong was renamed to PersistentLongFile.
# This line kind of creeps me out in the constructor of the Journal class. Maybe make a no-args
version of Storage#getStorageDir that asserts there's only one dir?
{code}
File currentDir = storage.getStorageDir(0).getCurrentDir();
{code}
# In general this patch seems to be mixing in protobufs in a few places where non-proto classes
seem more appropriate, notably in the Journal and JournalNodeRpcServer classes. Perhaps we
should create non-proto analogs for these protos and add translator methods?
# This seems really goofy. Just make another non-proto class and use a translator?
{code}
// Return the partial builder instead of the proto, since
{code}
# I notice that there's a few TODOs left in this patch. It would be useful to know which of
these you think need to be fixed before we commit this for real, versus those you'd like to
leave in and do as follow-ups.
# Instead of putting all of these classes in the o.a.h.hdfs.qjournal packages, I recommend
you try to separate these out into o.a.h.hdfs.qjoural.client, which implements the NN side
of things, and o.a.h.hdfs.qjournal.server, which implements the JN side of things. I think
doing so would make it easier to navigate the code.
# Could definitely use some method comments in the Journal class.
# Recommend renaming Journal#journal to something like Journal#logEdits or Journal#writeEdits.
# In JournalNode#getOrCreateJournal, this log message could be more helpful: LOG.info("logDir:
" + logDir);
# Seems like all of the timeouts in QuorumJournalManager should be configurable.
# I think you already have the config key to address this TODO in QJournalProtocolPB: // TODO:
need to add a new principal for loggers
# s/BackupNode/JournalNode/g:
{code}
+ * Protocol used to journal edits to a remote node. Currently,
+ * this is used to publish edits from the NameNode to a BackupNode.
{code}
# Use an HTML comment in journalstatus.jsp, instead of Java comments within a code block.
# Could use some more content for the journalstatus.jsp page. :)
# A few spots in the tests you catch expected IOEs, but don't verify that you received the
IOE you
 actually expect.
# Really solid tests overall, but how about one that actually works with HA? You currently
have a test for two entirely separate NNs, but not one that uses an HA mini cluster.
                
> Quorum-based protocol for reading and writing edit logs
> -------------------------------------------------------
>
>                 Key: HDFS-3077
>                 URL: https://issues.apache.org/jira/browse/HDFS-3077
>             Project: Hadoop HDFS
>          Issue Type: New Feature
>          Components: ha, name-node
>            Reporter: Todd Lipcon
>            Assignee: Todd Lipcon
>         Attachments: hdfs-3077-partial.txt, hdfs-3077.txt, hdfs-3077.txt, qjournal-design.pdf,
qjournal-design.pdf
>
>
> Currently, one of the weak points of the HA design is that it relies on shared storage
such as an NFS filer for the shared edit log. One alternative that has been proposed is to
depend on BookKeeper, a ZooKeeper subproject which provides a highly available replicated
edit log on commodity hardware. This JIRA is to implement another alternative, based on a
quorum commit protocol, integrated more tightly in HDFS and with the requirements driven only
by HDFS's needs rather than more generic use cases. More details to follow.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message