[ 
https://issues.apache.org/jira/browse/HDFS-13762?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16772797#comment-16772797
 ] 

Uma Maheswara Rao G commented on HDFS-13762:
--------------------------------------------

Thanks [~PhiloHe] for updating the patch. Please find the following feedback.
I am still reviewing it and will post remaining comments tomorrow.

#  {code:java}
   this.pmemManager = new PmemVolumeManager();
      this.pmemManager.load(pmemVolumes);
      PmemMappedBlock.setPersistentMemoryManager(this.pmemManager);
      PmemMappedBlock.setDataset(dataset);
{code}
  I think interface design can be cleaner. Now we have interfaces for 
MappableBlock but we really will not depend much on interface, we actually 
directly use class to load block as they are staic methods.
     I am thinking something like this: Lets have MemoryVolumeManager interface 
which will be implemented by two separate impelemnattions: PMemVolumeManager 
and MemMappedVolumeManager
     They can have init methods and getMappableBlockLoader
     They can return respective MappableBlockLoader classes, they are 
PMemMappedBlock or MemoryMappedBlock
     Then this loader will have interfaces of loadBlock, getLength, and 
afterCache. Lets make current static load methods can be provate static methods 
in PMemMappedBlock and MemoryMappedBlock and use them in loadBlock interface 
implemenattion.
     Does this make sense to you?
This will avoid the if check every time at 
{code:java}
if (pmemManager == null) {
            mappableBlock = MemoryMappedBlock.load(length, blockIn, metaIn,
                blockFileName, key);
          } else {
            mappableBlock = PmemMappedBlock.load(length, blockIn, metaIn,
                blockFileName, key);
          }
{code}
 MemoryVolumeManager#getMappableBlockLoader will get right loader and load the 
block.
#  "Fail to msync region. --> Failed to msync region
# // Load Intel ISA-L : you may want to removed this in pmdk file. I saw in 
couple of other places referring to ISA-L. Please remove them as its not 
related to that.
#  If some DNs configured with pmem volumes and some not, then blocks will be 
cached in both mmapped and pmemmapped? Probably you would like to document your 
recommendation?
# You may want to update the documentation for this feature 
https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/CentralizedCacheManagement.html
# Since its persistet, how does this handle or remember cached blocks when DN 
restart? 
# Do you want to have different metrics for different cache types? I am not 
sure its really necessary, but people may be interested to know how many blocks 
cached in pmem area? (Low priority)
# 
{noformat}
Build FAILED.
"C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.sln"
 (default target) (1) ->
"C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.vcxproj"
 (default target) (2) ->
(ClCompile target) ->
  src\org\apache\hadoop\util\bulk_crc32.c(59): warning C4091: 'static ' : 
ignored on left of 'int' when no variable is declared 
[C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.vcxproj]
"C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.sln"
 (default target) (1) ->
"C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.vcxproj"
 (default target) (2) ->
(ClCompile target) ->
  
c:\users\umgangum\work\hadoop\hadoop-common-project\hadoop-common\src\main\native\src\org\apache\hadoop\io\nativeio\pmdk_load.h(52):
 error C2061: syntax error : identifier '__d_pmem_map_file' 
[C:\Users\umgangum\Work\hadoop\hadoop-common-project\hadoop-common\src\main\native\native.vcxproj]
{noformat}
You need to fix for Windows as well. Am I missing something?
# From pmdk_load.h
  {code}
   __d_pmem_map_file pmem_map_file;
  __d_pmem_unmap pmem_unmap;
  __d_pmem_is_pmem pmem_is_pmem;
  __d_pmem_drain pmem_drain;
  __d_pmem_memcpy_nodrain pmem_memcpy_nodrain;
  __d_pmem_msync pmem_msync;
  {code}
  Quick question does this types available if non-unix env? I think you did 
tydef in Unix env only right


> Support non-volatile storage class memory(SCM) in HDFS cache directives
> -----------------------------------------------------------------------
>
>                 Key: HDFS-13762
>                 URL: https://issues.apache.org/jira/browse/HDFS-13762
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>          Components: caching, datanode
>            Reporter: Sammi Chen
>            Assignee: Feilong He
>            Priority: Major
>         Attachments: HDFS-13762.000.patch, HDFS-13762.001.patch, 
> HDFS-13762.002.patch, HDFS-13762.003.patch, HDFS-13762.004.patch, 
> HDFS-13762.005.patch, HDFS-13762.006.patch, HDFS-13762.007.patch, 
> HDFS-13762.008.patch, SCMCacheDesign-2018-11-08.pdf, SCMCacheTestPlan.pdf
>
>
> No-volatile storage class memory is a type of memory that can keep the data 
> content after power failure or between the power cycle. Non-volatile storage 
> class memory device usually has near access speed as memory DIMM while has 
> lower cost than memory.  So today It is usually used as a supplement to 
> memory to hold long tern persistent data, such as data in cache. 
> Currently in HDFS, we have OS page cache backed read only cache and RAMDISK 
> based lazy write cache.  Non-volatile memory suits for both these functions. 
> This Jira aims to enable storage class memory first in read cache. Although 
> storage class memory has non-volatile characteristics, to keep the same 
> behavior as current read only cache, we don't use its persistent 
> characteristics currently.  
>  
>  
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to