[GitHub] [hadoop] liuml07 commented on a change in pull request #2189: HDFS-15025. Applying NVDIMM storage media to HDFS

[GitBox <gi...@apache.org>]

liuml07 commented on a change in pull request #2189:
URL: https://github.com/apache/hadoop/pull/2189#discussion_r475381174



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/ArchivalStorage.md
##########
@@ -27,15 +27,17 @@ The frameworks provided by Heterogeneous Storage and Archival Storage generalize
 Storage Types and Storage Policies
 ----------------------------------
 
-### Storage Types: ARCHIVE, DISK, SSD and RAM\_DISK
+### Storage Types: ARCHIVE, DISK, SSD, NVDIMM and RAM\_DISK
 
 The first phase of [Heterogeneous Storage (HDFS-2832)](https://issues.apache.org/jira/browse/HDFS-2832) changed datanode storage model from a single storage, which may correspond to multiple physical storage medias, to a collection of storages with each storage corresponding to a physical storage media. It also added the notion of storage types, DISK and SSD, where DISK is the default storage type.
 
 A new storage type *ARCHIVE*, which has high storage density (petabyte of storage) but little compute power, is added for supporting archival storage.
 
 Another new storage type *RAM\_DISK* is added for supporting writing single replica files in memory.
 
-### Storage Policies: Hot, Warm, Cold, All\_SSD, One\_SSD, Lazy\_Persist and Provided
+And a new storage type *NVDIMM*, which is a non-volatile memory that will retains the datastored on the memory when the computer is powered down or system crashes and restore the data when the machine powered on, is added for supporting archival storage.

Review comment:
       1. `is added for supporting archival storage.` What is the main purpose of this use case? I don't believe this is for "archival storage".
   1. Also, this sentence is a bit verbose to me. Do you think we can make it concise, something like this?
     ```
     From Hadoop 3.4, a new storage type *NVDIMM* is added for supporting writing replica files in non-volatile 
     memory that has the capability to hold saved data even if the power is turned off.
     ```
     If you instead prefer keeping the current phrase, correct the syntax error by `s/retains/retain/` and `s/datastored/data stored`.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockStoragePolicySuite.java
##########
@@ -63,6 +63,12 @@ public static BlockStoragePolicySuite createDefaultSuite(
         new StorageType[]{StorageType.DISK},
         new StorageType[]{StorageType.DISK},
         true);    // Cannot be changed on regular files, but inherited.
+    final byte allNVDIMMId = HdfsConstants.StoragePolicy.ALL_NVDIMM.value();

Review comment:
       nit: I see other variables are using lower case, could we change this name to `allnvdimmId`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org