HDFS-16043. Add markedDeleteBlockScrubberThread to delete blocks asynchronously #3063
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 19, 2021
There was a problem hiding this comment.
Thanks @zhuxiangyi for your works here. It is very interesting improvement. Leave some minor comment inline. PTAL.
cc @jojochuang @ayushtkn @goiri Would you mind give another checks?
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
Outdated
Show resolved
Hide resolved
...oject/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFileTruncate.java
Outdated
Show resolved
Hide resolved
...ect/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeMXBean.java
Outdated
Show resolved
Hide resolved
|
@zhuxiangyi This PR has conflict to trunk now. Please rebase it first and check failed unit test if it is related to changes. Thanks. |
zhuxiangyi
force-pushed
the
HDFS-16043
branch
2 times, most recently
from
3bee0e3 to
a92b060
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao Thank you for your review, the above check failure seems to have nothing to do with this patch. |
|
Please fix the checkstyle reported by Yetus first. Other looks good to me. I would like to give my +1 when fix that. Before commit to trunk we should wait for other guys give another reviews. Thanks @zhuxiangyi . |
|
💔 -1 overall
This message was automatically generated. |
jojochuang
requested changes
Jun 25, 2021
There was a problem hiding this comment.
Thanks for the work and sharing the flame graph, which makes it easy to validate the change.
However, I am still not able to understand why the change improves delete performance. The delete op is done in two steps, step 1 acquire lock, collect blocks, release lock. step 2 acquire lock, delete blocks, release lock.
The change essentially moves the step2 to another thread. IMO, this approach reduces client perceived latency, which is good. But deleting the blocks still requires holding namespace lock. Why does it avoid NN unresponsiveness?
Is it because instead of releasing the lock after a specified number of blocks, it releases the lock after an absolute time. I can image the absolute time is a better metric because deleting a block does take a variable duration of time, not constant.
A few minor comments changes requested:
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Outdated
Show resolved
Hide resolved
jojochuang
reviewed
Jun 25, 2021
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
@jojochuang Thanks for your comment and review, as you commented, the current modification is only to delete the block asynchronously. The QuotaCount calculation optimization described in jira can reduce the time to collect blocks. I plan to open a new problem to solve it. |
|
💔 -1 overall
This message was automatically generated. |
jojochuang
requested changes
Sep 23, 2021
zhuxiangyi
force-pushed
the
HDFS-16043
branch
from
53f1dfb to
20633a5
Compare
@jojochuang already rebase. |
jianghuazhu
reviewed
Sep 23, 2021
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
jianghuazhu
reviewed
Sep 26, 2021
...ct/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
Outdated
Show resolved
Hide resolved
zhuxiangyi
force-pushed
the
HDFS-16043
branch
from
35ddddb to
e9999ac
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Can we update the subject of this PR? As mentioned in a previous comment this PR makes block removal asynchronous and does actually speed up delete. #3063 (comment) |
zhuxiangyi
changed the title
tomscut
approved these changes
Jan 9, 2022
Hexiaoqiao
approved these changes
Jan 9, 2022
There was a problem hiding this comment.
LGTM. +1 from my side.
Please consider the following improvements at next loop.
A. Just notice that there are several other operations also invoke remove Block as Wei-Chiu mentioned above, I think we should process them asynchronously.
B. Pending deletion blocks number will be concerned by end user IMO, it is better to add it as another metric index.
Thanks @zhuxiangyi for your works!
|
Will commit to trunk wait for two days if no more other comments. |
zhuxiangyi
force-pushed
the
HDFS-16043
branch
from
91c8940 to
108e96c
Compare
|
Added pendingDeleteBlocksCount metrics please review @Hexiaoqiao |
|
💔 -1 overall
This message was automatically generated. |
zhuxiangyi
force-pushed
the
HDFS-16043
branch
from
108e96c to
0d1f846
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
Will commit to trunk once jenkins completed. |
|
🎊 +1 overall
This message was automatically generated. |
|
@zhuxiangyi Do you mind to fix the checkstyle refer to https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3063/19/artifact/out/results-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt. Thanks. |
|
@Hexiaoqiao resubmitted |
|
Commit to trunk based on build result (#3063 (comment)) and checkstyle fix only(f6f2793). Thanks @zhuxiangyi for your contribution. Thanks @jojochuang and @tomscut for you reviews. |
|
@zhuxiangyi It is conflict when cherry-pick to other active branches. Do you mind to create another PR to branch-3.3? |
|
ok, I will submit it later. |
|
💔 -1 overall
This message was automatically generated. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…chronously (apache#3063). Contributed by Xiangyi Zhu. Reviewed-by: tomscut <[email protected]> Signed-off-by: Wei-Chiu Chuang <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
HDFS-16043