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

Todd Lipcon commented on HDFS-4350:
-----------------------------------

{code}
+   * Set the value of {@link DatanodeManager#isAvoidingStaleDataNodesForWrite}.
+   * The HeartbeatManager disables write avoidance when more than
+   * dfs.namenode.write.stale.datanode.ratio of DataNodes are marked as stale.
{code}

The double negative here is confusing. I think better to say: "The 
HeartbeatManager will allow writes to stale datanodes when more than ..."

----

{code}
+  boolean getCheckForStaleDataNodes() {
{code}

I think for a boolean-type return value, something like 
{{shouldCheckForStale...} is better than {{getCheckForStale...}}

----
{code}
   HeartbeatManager(final Namesystem namesystem,
-      final BlockManager blockManager, final Configuration conf) {
+      final BlockManager blockManager, final Configuration conf,
+      final boolean avoidStaleDataNodesForWrite, final long staleInterval) {
{code}

Not a fan of proliferating constructor parameters here. Since we already have 
the conf, and those two new parameters just come from the conf, I think the 
earlier approach was better (having both classes access the conf).

----
{code}
+      this.heartbeatRecheckInterval = staleInterval;
+      LOG.info("Setting hearbeat interval to " + staleInterval
+          + " since dfs.namenode.stale.datanode.interval < "
+          + "dfs.namenode.heartbeat.recheck-interval");
{code}

This info message is a little unclear -- the heartbeat interval isn't actually 
being changed, just the interval at which the HeartbeatManager wakes up to 
check for expired DNs. The message makes it sound like the datanodes will 
heartbeat more often, but in fact it's only an NN-side frequency that's being 
clamped down.

Also, please interpolate the {{BLAH_BLAH_KEY}} constants instead of actually 
writing the configuration keys in the log message.

----
{code}
+    Indicate whether or not to avoid reading from "stale" datanodes whose
{code}
Should use &quot; here, for valid XML.

                
> Make enabling of stale marking on read and write paths independent
> ------------------------------------------------------------------
>
>                 Key: HDFS-4350
>                 URL: https://issues.apache.org/jira/browse/HDFS-4350
>             Project: Hadoop HDFS
>          Issue Type: Bug
>    Affects Versions: 3.0.0
>            Reporter: Andrew Wang
>            Assignee: Andrew Wang
>         Attachments: hdfs-4350-1.patch, hdfs-4350-2.patch
>
>
> Marking of datanodes as stale for the read and write path was introduced in 
> HDFS-3703 and HDFS-3912 respectively. This is enabled using two new keys, 
> {{DFS_NAMENODE_CHECK_STALE_DATANODE_KEY}} and 
> {{DFS_NAMENODE_AVOID_STALE_DATANODE_FOR_WRITE_KEY}}. However, there currently 
> exists a dependency, since you cannot enable write marking without also 
> enabling read marking, since the first key enables both checking of staleness 
> and read marking.
> I propose renaming the first key to 
> {{DFS_NAMENODE_AVOID_STALE_DATANODE_FOR_READ_KEY}}, and make checking enabled 
> if either of the keys are set. This will allow read and write marking to be 
> enabled independently.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to