[
https://issues.apache.org/jira/browse/HDFS-14355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16805938#comment-16805938
]
Feilong He commented on HDFS-14355:
-----------------------------------
Thanks [~anoop.hbase] for your valuable suggestions. We have uploaded a new
patch HDFS-14355.009.patch with some updates based on your suggestions.
{quote}getBlockInputStreamWithCheckingPmemCache -> Can be private method
{quote}
Yes, this method can be private and it's better to make it private. We have
fixed this issue in the new patch.
{quote}public PmemVolumeManager getPmemVolumeManager -> Why being exposed? For
tests? If so can this be package private? And also mark it with
@VisibleForTesting
{quote}
Indeed, there is no need to expose this method in the current impl. It should
be package private with @VisibleForTesting annotation. This issue has been
fixed in the new patch.
{quote}I think the afterCache() thing is an unwanted indirection
Actually in PmemMappableBlockLoader#load, once the load is successful
(mappableBlock != null), we can do this pmemVolumeManager work right?
{quote}
Good suggestion. As you pointed, using #afterCache() is indirect and such impl
can easily cause bugs. In the new patch, afterCache work will be executed after
a mapppableBlock is successfually loaded.
{quote}Call afterUncache() after delete the file
{quote}
Yes, #afterUncache should be executed after file is deleted.
{quote}public PmemVolumeManager(DNConf dnConf)
Can we only pass pmemVolumes and maxLockedPmem? That is cleaner IMO
{quote}
Another good suggestion. PmemVolumeManager actually just needs pomemVolumes and
maxLoackedPmem for instantiation.
{quote}getVolumeByIndex -> can this be package private
{quote}
Yes, package private is enough. We have modified the access specifier in the
new patch.
{quote}getCacheFilePath(ExtendedBlockId key) -> Better name would be
getCachedPath(ExtendedBlockId)
{quote}
Yes, the method name getCacheFilePath is a bit ambiguous. In the new patch,
this method is named as getCachedPath as you suggested.
{quote}dfs.datanode.cache.pmem.capacity -> Am not sure any naming convention u
follow in HDFS. But as a user I would prefer a name
dfs.datanode.pmem.cache.capacity. Ditto for dfs.datanode.cache.pmem.dirs
{quote}
“dfs.datanode.cache” is the prefix followed for all the cache related configs,
so we would like to follow the pattern.
Thanks [~anoop.hbase] again for your comments.
> Implement HDFS cache on SCM by using pure java mapped byte buffer
> -----------------------------------------------------------------
>
> Key: HDFS-14355
> URL: https://issues.apache.org/jira/browse/HDFS-14355
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: caching, datanode
> Reporter: Feilong He
> Assignee: Feilong He
> Priority: Major
> Attachments: HDFS-14355.000.patch, HDFS-14355.001.patch,
> HDFS-14355.002.patch, HDFS-14355.003.patch, HDFS-14355.004.patch,
> HDFS-14355.005.patch, HDFS-14355.006.patch, HDFS-14355.007.patch,
> HDFS-14355.008.patch, HDFS-14355.009.patch
>
>
> This task is to implement the caching to persistent memory using pure
> {{java.nio.MappedByteBuffer}}, which could be useful in case native support
> isn't available or convenient in some environments or platforms.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
