Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 3.0.0
-
Fix Version/s: 1.2.0, 2.0.3-alpha
-
Component/s: None
-
Labels:None
-
Hadoop Flags:Incompatible change, Reviewed
-
Release Note:
-
Target Version/s:
Description
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.
Issue Links
Activity
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12562980/hdfs-4350-1.patch
against trunk revision .
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3714//console
This message is automatically generated.
Tried to bump Jenkins (cancel patch, upload, submit patch?). I think it's currently ready for review though.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563592/hdfs-4350-1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
-1 javac. The patch appears to cause the build to fail.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3749//console
This message is automatically generated.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563592/hdfs-4350-1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
-1 javac. The patch appears to cause the build to fail.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3750//console
This message is automatically generated.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563594/hdfs-4350-1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3751//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3751//console
This message is automatically generated.
+ * 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.
The double negative here is confusing. I think better to say: "The HeartbeatManager will allow writes to stale datanodes when more than ..."
+ boolean getCheckForStaleDataNodes() {
I think for a boolean-type return value, something like shouldCheckForStale...} is better than {{getCheckForStale...
HeartbeatManager(final Namesystem namesystem, - final BlockManager blockManager, final Configuration conf) { + final BlockManager blockManager, final Configuration conf, + final boolean avoidStaleDataNodesForWrite, final long staleInterval) {
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).
+ this.heartbeatRecheckInterval = staleInterval; + LOG.info("Setting hearbeat interval to " + staleInterval + + " since dfs.namenode.stale.datanode.interval < " + + "dfs.namenode.heartbeat.recheck-interval");
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.
+ Indicate whether or not to avoid reading from "stale" datanodes whose
Should use " here, for valid XML.
Thanks for the review Todd, think I hit all your feedback. Config strings are still unfortunately hardcoded in the javadoc comments, but I don't know how to avoid that.
Config strings are still unfortunately hardcoded in the javadoc comments
How about using
{@link DFSConfigKeys#xxxx_KEY}?
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563793/hdfs-4350-3.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3797//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3797//console
This message is automatically generated.
Switched the config strings in javadoc over to @link, based on Jing's advice.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12564544/hdfs-4350-4.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3829//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3829//console
This message is automatically generated.
Hey Andrew. I took another look at this and think it can be cleaned up a bit more... having two separate booleans (and respective getters) for isAvoidingStaleDataNodesForWrite vs shouldAvoidStaleDatanodesForWrite is pretty messy.
What if we did the following?
- kill off the boolean for isAvoidingStaleDatanodesForWrite
- change heartbeatCheck to always check stale nodes. It seems like there is practically zero cost to this. In the usual case where the heartbeat check does not find a dead node, we already iterate over the whole list of nodes. Checking isStale() is just a comparison, so essentially free.
- then DatanodeManager only needs to expose the current state: shouldAvoidStaleDataNodesForWrite, which would be implemented something like":
return avoidStaleDatanodesForWrite && numStaleNodes < datanodeMap.size() * ratioUseStaleDatanodesForWrite
(or still keep a cached boolean inside DatanodeManager, but set inside setNumStaleNodes)
That also allows you to move the ratio into DatanodeManager, which makes more sense in my opinion. I think all these changes together could allow you to remove the reference backward from HeartbeatManager into DatanodeManager - the circular reference between the classes is the thing that smells bad to me.
Andrew's on vacation, so I took a whack at implementing the above suggestion against his patch. This patch seems to pass the TestReplicationPolicy tests. I also ran that test through jcarder to double check there weren't any lock inversions.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12565057/hdfs-4350.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:
org.apache.hadoop.hdfs.server.blockmanagement.TestBlockManager
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3845//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3845//console
This message is automatically generated.
Todd's patch looks good to me. I ran the failed tests a couple times locally and they passed, and earlier run on this jira were fine.
Patch looks pretty good to me, and I agree that the test failure seems unrelated. One little comment, this doesn't parse so well:
// When the number stale datanodes marked as stale reaches this ratio,
+1 once this is addressed.
This change was made available in 2.0.2-alpha and has been backported to 1.1.0. So changing the configuration name and semantics will have impact on those releases. How is it going to be handled?
The patch also looks good to me. One minor issue: the unused "import com.google.common.base.Preconditions;" can be removed from HeartbeatManager.java.
Thanks for reviews, Aaron and Jing. Improved the comment and removed the extra import, and axed some trailing whitespace.
Suresh, what's our policy on changing config strings and behavior for alpha and beta releases? The current name would be misleading, and it'd be kind of unfortunate to have to deprecate it already.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12566107/hdfs-4350-5.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3872//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3872//console
This message is automatically generated.
Suresh, what's our policy on changing config strings and behavior for alpha and beta releases? The current name would be misleading, and it'd be kind of unfortunate to have to deprecate it already.
I think this is a good change. Out of curiosity, is there a use case for using stale datanode information for write path alone (which is what currently not supported).
Changing configuration name and semantics will be an incompatible change. I am okay making the assumption that this was used only for HBase MTTR work for now and make these changes both in branch-2 and branch-2. For branch-1, it is good to loop in the release manager Matt Foley. Given 2.0.2 is marked alpha, I am okay letting this incompatible change into 2.0.3.
Additionally, related change needs to go into HBase as well, related to HBASE-5843. Not sure the backward compatibility impact on HBase.
One additional comment - if the config name is changed, it should go into both branch-1 and branch-2. That way the migration from 1.x release to 2.x is not complicated due to the need to change config name.
For HBase, it's not a big deal:
- in all case, both write & read path needs to use the stale mode.
- the burden is for the HDFS admin, as it needs to be configured in HDFS.
Hey Suresh, plan sounds good. I'll spin up a branch-1 patch and ping Matt about it. I don't have a usecase for making these independent, but it's nice to remove the configuration dependency and make the name more clear.
My understanding is that no code changes should be needed on the HBase side, but perhaps Nicolas Liochon can confirm.
branch-1 backport posted. Tested via running TestReplicationPolicy and TestGetBlocks. Also slightly updated the existing trunk patch with more explicit heartbeatChecks in TestReplicationPolicy.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12566226/hdfs-4350-6.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:
org.apache.hadoop.hdfs.server.blockmanagement.TestReplicationPolicy
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3876//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3876//console
This message is automatically generated.
yep, I confirm there is no code change in HBase.
Rev both patches. We were incorrectly using < instead of <= when doing the ratio comparison, which made the HalfStale test fail. Tested by running TestReplicationPolicy for both.
Marking this jira incompatible.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12566341/hdfs-4350-7.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3877//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3877//console
This message is automatically generated.
I think this is a good change, although introducing a minor incompatibility.
1.2 will have other incompatible changes, so it should be okay to add this to the branch-1 line.
Please do not put it in the branch-1.1 line. Thanks.
Andrew Wang +1 for the trunk patch. I will review the branch-1 patch shortly.
The branch-1 patch looks good to me. One minor issue: FSNamesystem#getRatioUseStaleNodesForWriteFromConf is never used. You may either use it to get the value of ratioUseStaleDataNodesForWrite from configuration, or remove it but add some similar value checking for ratioUseStaleDataNodesForWrite.
Besides, I've run ant test locally for the patch, and all the tests passed except TestSubmitJob and TestJvmReuse, which should be un-related.
Nice catch Jing. I changed it to use the static function. Ran TestReplicationPolicy,TestGetBlocks which passed fine.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12567307/hdfs-4350-branch-1-3.patch
against trunk revision .
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3914//console
This message is automatically generated.
+1 for the branch-1 patch as well.
Integrated in Hadoop-trunk-Commit #3315 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3315/)
HDFS-4350. Make enabling of stale marking on read and write paths independent. Contributed by Andrew Wang. (Revision 1441819)
Result = SUCCESS
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1441819
Files :
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/HeartbeatManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/metrics/TestNameNodeMetrics.java
I committed the patch to trunk, branch-2 and branch-1.
Thank you Andrew!
Integrated in Hadoop-Yarn-trunk #116 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/116/)
HDFS-4350. Make enabling of stale marking on read and write paths independent. Contributed by Andrew Wang. (Revision 1441819)
Result = SUCCESS
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1441819
Files :
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/HeartbeatManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/metrics/TestNameNodeMetrics.java
Integrated in Hadoop-Hdfs-trunk #1305 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1305/)
HDFS-4350. Make enabling of stale marking on read and write paths independent. Contributed by Andrew Wang. (Revision 1441819)
Result = SUCCESS
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1441819
Files :
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/HeartbeatManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/metrics/TestNameNodeMetrics.java
Integrated in Hadoop-Mapreduce-trunk #1333 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1333/)
HDFS-4350. Make enabling of stale marking on read and write paths independent. Contributed by Andrew Wang. (Revision 1441819)
Result = FAILURE
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1441819
Files :
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/HeartbeatManager.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestGetBlocks.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicy.java
- /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/metrics/TestNameNodeMetrics.java
Patch makes the configurations independent. Did some refactors along the way to avoid reading config parameters in multiple places, so the patch actually removes more lines than it adds. Some renaming for consistency.
I based this off of the patch in
HDFS-4351, but my `git apply --check` indicates it applies to trunk okay too.