HDFS-15548. Allow configuring DISK/ARCHIVE storage types on same device mount #2288
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
Sep 17, 2020
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Thanks @LeonGao91 for your works, some comment inline.
I wonder that if someone could config more than one archive path at one device (for some reason or mis-config), then it may not work correct, right? Which works fine for logic disk in my opinion although it is not recommended. Thanks.
...p-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeImpl.java
Outdated
Show resolved
Hide resolved
| private final DataNodeVolumeMetrics metrics; | ||
| private URI baseURI; | ||
| private boolean enableSameDiskArchival; | ||
| private final String device; |
Contributor
There was a problem hiding this comment.
what about using storageID replace device? IMO both of them are in order to index single volume, right?
Contributor
Author
There was a problem hiding this comment.
The "device" here is the string value of the filesystem mount point. I wanted to use it to keep track of which two volumes are on the same mount (thus the same disk). Datanode can use the existing DF#getMount() to detect it automatically.
I can probably change the name to "mount" to make it more clear.
| if (reservedForArchive >= 1) { | ||
| FsDatasetImpl.LOG.warn("Value of reserve-for-archival is >= 100% for " | ||
| + currentDir + ". Setting it to 99%."); | ||
| reservedForArchive = 0.99; |
Contributor
There was a problem hiding this comment.
Why reservedForArchive has to less than 1 here, IIUC it means that this is ARCHIVE device when reservedForArchive set to 1. Right?
Contributor
Author
There was a problem hiding this comment.
Yeah I think you are right, I will update and make this at most 1.
Contributor
Author
Thanks for the review! @Hexiaoqiao I think this feature is mostly useful if users don't want to setup Linux level partitions to divide DISK/ARCHIVE, in which the size of partitions is difficult to change in production. For the question:
|
Contributor
Thanks @LeonGao91 , this is indeed my concern, I think only log is not proper way, because the following logic will be not correct, especially the capacity remains if mis-config. IMO, DataNode instance exit probably more graceful. FYI, Thanks. |
Contributor
Author
That makes sense, I will make the change accordingly. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…created on same mount
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@Hexiaoqiao Would you please take a second look? I have added a check as we discussed with UT. |
Contributor
|
Sorry, I missed this message before. Will have another review later today. Thanks. |
Hexiaoqiao
reviewed
Sep 24, 2020
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Thanks @LeonGao91 for your works, some comment inline.
...p-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeList.java
Outdated
Show resolved
Hide resolved
| DFSConfigKeys.DFS_DATANODE_ALLOW_SAME_DISK_TIERING_DEFAULT); | ||
| if (enableSameDiskArchival) { | ||
| this.mount = usage.getMount(); | ||
| reservedForArchive = conf.getDouble( |
Contributor
There was a problem hiding this comment.
reservedForArchive try to define reserve for archive percentage. If there are heterogeneous disks located one node, do we need config them separate？
Contributor
Author
There was a problem hiding this comment.
Yeah, it's a good point. The reason I put it this way is to make configuration less verbose for normal use cases that datanode only has one type of disk. Otherwise, users will need to tag all the disks which is less readable and easy to make mistakes.
I think we can introduce additional config for the use case you mentioned later, to list out each volume and target ratio.
| capacity = configuredCapacity; | ||
| } | ||
|
|
||
| if (enableSameDiskArchival) { |
Contributor
There was a problem hiding this comment.
The return value seems not expected as annotation says if enable this feature.
the capacity of the file system excluding space reserved for non-HDFS.
IMO, the part for ARCHIVE should also be calculated. It seems be not differentiated by NameNode for DISK or ARCHIVE per storage of DataNode. Please correct if something wrong.
Contributor
Author
There was a problem hiding this comment.
This is actually the important part to enable this feature, to allow users to configure the capacity of a fsVolume.
As we are configuring two fsVolume on the same underlying filesystem, if we do nothing the capacity will be calculated twice thus all the stats being reported will be incorrect.
Here is an example:
Let's say we want to configure [DISK]/data01/dfs and [ARCHIVE]/data01/dfs_archive on a 4TB disk mount /data01, and we want to assign 1 TB to [DISK]/data01/dfs and 3 TB for [ARCHIVE]/data01/dfs_archive, we can make reservedForArchive to be 0.75 and put those two dirs in the volume list.
In this case, /data01/dfs will be reported as a 1TB volume and /data01/dfs_archive will be reported as 3TB volume to HDFS. Logically, HDFS will just treat them as two separate volumes.
If we don't make the change here, HDFS will see two volumes and each of them is 4TB, in that case, the 4TB disk will be counted as 4 * 2 = 8TB capacity in namenode and all the related stats will be wrong.
Another change we need to make is the getActualNonDfsUsed() as below. Let's say in the above 4TB disk setup we use 0.1TB as reserved, and [ARCHIVE]/data01/dfs_archive already has 2TB capacity used, in this case when we are calculating the getActualNonDfsUsed() for [DISK]/data01/dfs it will always return 0, which is not correct and it will cause other weird issues. As the two fsVolumes are on the same filesystem, the reserved space should be shared.
According to our analysis and cluster testing result, updating these two functions getCapacity() and getActualNonDfsUsed() is enough to keep stats correct for the two "logical" fsVolumes on same disk.
I can update the java doc to reflect this when the feature is turned on.
Contributor
There was a problem hiding this comment.
Thanks @LeonGao91 for your detailed comments. It makes sense for me.
...p-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsVolumeImpl.java
Show resolved
Hide resolved
Contributor
Author
|
@Hexiaoqiao Thanks for the comments! I have replied and please let me know if it makes sense to you. |
Contributor
|
+1 from my side. Ping @jojochuang @ayushtkn @goiri would you like to have another check? Thanks |
Contributor
Author
|
Thanks @Jing9 For the review!
|
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Jing9
reviewed
Oct 30, 2020
| // Beware that capacity usage might be >100% if there are already | ||
| // data blocks exist and the configured ratio is small, which will | ||
| // prevent the volume from taking new blocks until capacity is balanced out. | ||
| public static final String DFS_DATANODE_RESERVE_FOR_ARCHIVE_PERCENTAGE = |
Contributor
There was a problem hiding this comment.
Take another look at the patch, I think it may be better to have the percentage as a tag added to the configuration "dfs.datanode.data.dir", just following the storage type tag. In this way on the same datanode we can have different percentage settings for different mount points. What do you think?
Contributor
Author
There was a problem hiding this comment.
The intention is to have a configuration as a "default value" for all disks, as in normal cases one datanode server comes with the same type of HDDs. Therefore we can keep the DN configuration less verbose for most of the use cases.
However, you are right that we should allow users to configure different values, and it is a good idea to put it under "dfs.datanode.data.dir".
I will create a follow-up JIRA to address it, so we can keep this PR from being too big, as that could involve quite some change.
| if (volume.getStorageType() == StorageType.ARCHIVE) { | ||
| volumeInfo.setCapacityRatio(reservedForArchive); | ||
| } else if (volume.getStorageType() == StorageType.DISK) { | ||
| volumeInfo.setCapacityRatio(1 - reservedForArchive); |
Contributor
There was a problem hiding this comment.
what if we have a mount with one single volume? Following the current implementation we may assign an unnecessary capacity ratio to it. We only need to calculate and assign the ratio for volumes sharing the same mount with others.
Contributor
Author
There was a problem hiding this comment.
Thats a good point. I will make the change to ignore the capacity ratio of the volume if there is only one on the mount.
|
💔 -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. |
Jing9
reviewed
Nov 3, 2020
Contributor
Jing9
left a comment
Jing9
left a comment
There was a problem hiding this comment.
Thanks for updating the patch, @LeonGao91 ! The current patch looks good to me except some minor issues. +1 after addressing the comments.
...dfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/MountVolumeInfo.java
Show resolved
Hide resolved
...dfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/MountVolumeInfo.java
Show resolved
Hide resolved
| */ | ||
| @InterfaceAudience.Private | ||
| class MountVolumeInfo { | ||
| private ConcurrentMap<StorageType, FsVolumeImpl> |
Contributor
There was a problem hiding this comment.
This field can be declared as "final"
...hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/MountVolumeMap.java
Show resolved
Hide resolved
| this.enableSameDiskTiering = | ||
| conf.getBoolean(DFSConfigKeys.DFS_DATANODE_ALLOW_SAME_DISK_TIERING, | ||
| DFSConfigKeys.DFS_DATANODE_ALLOW_SAME_DISK_TIERING_DEFAULT); | ||
| if (enableSameDiskTiering) { |
Contributor
There was a problem hiding this comment.
let's also add a check " && usage != null"
| private final BlockScanner blockScanner; | ||
|
|
||
| private boolean enableSameDiskTiering; | ||
| private MountVolumeMap mountVolumeMap; |
Contributor
There was a problem hiding this comment.
These two fields can be declared as "final"
| void addVolume(FsVolumeReference ref) { | ||
| FsVolumeImpl volume = (FsVolumeImpl) ref.getVolume(); | ||
| volumes.add(volume); | ||
| if (enableSameDiskTiering && |
Contributor
There was a problem hiding this comment.
maybe consider putting this check into a method
Contributor
Author
There was a problem hiding this comment.
Good idea, will do that
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Hi @Hexiaoqiao I have updated the PR with comments from Jing, please feel free to take a second look if you get a chance, thanks! |
Contributor
|
Thanks @LeonGao91 and @Jing9 for your works, will confirm today later. Thanks. |
Hexiaoqiao
reviewed
Nov 9, 2020
Contributor
Hexiaoqiao
left a comment
Hexiaoqiao
left a comment
There was a problem hiding this comment.
Thanks @LeonGao91 and @Jing9 for your works here. The patch looks good to me in general. Just leave minor comments. +1 after addressing. Thanks.
|
|
||
| MountVolumeInfo(Configuration conf) { | ||
| storageTypeVolumeMap = new ConcurrentHashMap<>(); | ||
| reservedForArchiveDefault = conf.getDouble( |
Contributor
There was a problem hiding this comment.
check if it set a negative value?
| } | ||
| } | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
codestyle: redundant empty line.
Contributor
Author
There was a problem hiding this comment.
Good catch, wonder if checksyle should catch it tho.
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute