HDFS-14318:dn cannot be recognized and must be restarted to recognize the Repaired disk #1104
+91
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hexiaoqiao
reviewed
Jul 16, 2019
There was a problem hiding this comment.
- Would you like to add new unit test to cover this update?
- It is better to submit single one PR for one improvement. I think this PR is same as HDFS-14318:dn cannot be recognized and must be restarted to recognize the Repaired disk #538 .
| @@ -404,6 +404,11 @@ public static InetSocketAddress createSocketAddr(String target) { | |||
| private final StorageLocationChecker storageLocationChecker; | |||
|
|
|||
| private final DatasetVolumeChecker volumeChecker; | |||
| volatile FsDatasetSpi<? extends FsVolumeSpi> allData = null; | |||
| private Thread checkDiskThread = null; | |||
| private final int checkDiskInterval = 5*1000; | |||
There was a problem hiding this comment.
it is better as one parameter to config.
| private Thread checkDiskThread = null; | ||
| private final int checkDiskInterval = 5*1000; | ||
| private Object checkDiskMutex = new Object(); | ||
| private List<StorageLocation> errorDisk = null; |
There was a problem hiding this comment.
allData = data + errorDisk right? It seems that allData is redundant variable.
There was a problem hiding this comment.
allData = data + errorDisk is right. allData will use in volumeChecker.checkAllVolumes(allData).
If no allData, we must make a variable before volumeChecker.checkAllVolumes, the variable is data + errorDisk. So I think allData is ok .
| @@ -819,6 +825,14 @@ public IOException call() { | |||
| } else { | |||
| effectiveVolumes.add(volume.toString()); | |||
| LOG.info("Successfully added volume: {}", volume); | |||
| if (errorDisk != null && !errorDisk.isEmpty()) { | |||
There was a problem hiding this comment.
It may be not necessary to check if errorDisk is empty.
There was a problem hiding this comment.
I don't think so. If refreshVolumes is invoked by reconfigurePropertyImpl, errorDisk may be null.
public String reconfigurePropertyImpl(String property, String newVal)
throws ReconfigurationException {
switch (property) {
case DFS_DATANODE_DATA_DIR_KEY: {
IOException rootException = null;
try {
LOG.info("Reconfiguring {} to {}", property, newVal);
this.refreshVolumes(newVal);
...
}| @@ -2191,6 +2208,44 @@ public void shutdown() { | |||
| tracer.close(); | |||
| } | |||
|
|
|||
| public void startCheckDiskThread() { | |||
There was a problem hiding this comment.
Suggest to use ScheduledExecutorService to schedule this task at fixed rate. since,
- same target,
- simpler,
checkDiskMutex, it seems to me that it it no need to concurrency control.
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Show resolved
Hide resolved
| errorDisk = new ArrayList<>(); | ||
| } | ||
| List<StorageLocation> tmpDisk = Lists.newArrayList(errorDisk); | ||
| errorDisk = null; |
There was a problem hiding this comment.
maybe this statement is unexpected.
There was a problem hiding this comment.
errorDisk=null should be errorDisk.clear()
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Show resolved
Hide resolved
|
Thanks @hunshenshi for reporting the issue and the PR. Agree it would be great if you can add a unit test that repro the issue. The Datanode has a disk volume checker that checks volumes every 15m (by default), you may use the callback to populate the healthy volumes and failed volumes. |
Thanks, I will fix that. |
|
@xiaoyuyao ok. I will read it. |
|
@Hexiaoqiao Do you have any idea for the UT? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao |
|
💔 -1 overall
This message was automatically generated. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
…ot reflected in code In the current code, the window is only applied and checked on the last retry. However, the check should be done at all retries. This was found during apache#1104 rmatharu please take a look Author: Daniel Nishimura <dnishimura@gmail.com> Reviewers: Ray Matharu <rmatharu@linkedin.com> Closes apache#1108 from dnishimura/samza-2277-cluster-manager-retry-window-bug-fix and squashes the following commits: 23db835e [Daniel Nishimura] Address minor comments from @rmatharu 0a7c9270 [Daniel Nishimura] Trigger build b/c of flakey test. 0c56499d [Daniel Nishimura] Fix checkstyle 2674f75e [Daniel Nishimura] Address @rmatharu's comments. ecfe63eb [Daniel Nishimura] SAMZA-2277: Semantics for cluster-manager.container.retry.window.ms not reflected in code
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
…host-affinity allocations (apache#1104) * SAMZA-2266: Introduce a backoff when there are repeated failures for host-affinity allocations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
dn detected that disk a has failed. After disk a is repaired, dn cannot be recognized and must be restarted to recognize
I make a patch to dn for recognize the repaired disk without restart dn