[
https://issues.apache.org/jira/browse/HDFS-14355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16803612#comment-16803612
]
Rakesh R edited comment on HDFS-14355 at 3/28/19 6:12 AM:
----------------------------------------------------------
Adding review comments, please take care.
# How about adding an API to interface
{{MappableBlockLoader#isTransientCache()}} to avoid checks specific to PMem. It
can return specific flag value to differentiate NVMe/DRAM based cache.
{code:java}
public boolean isPmemCacheEnabled() {
return mappableBlockLoader instanceof PmemMappableBlockLoader;
}
{code}
# I'd like to avoid type casting. It won't work with another Pmem
implementation, right?
{code:java}
public String getReplicaCachePath(String bpid, long blockId) {
if (!isPmemCacheEnabled() || !isCached(bpid, blockId)) {
return null;
}
ExtendedBlockId key = new ExtendedBlockId(blockId, bpid);
String cachePath = ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager()
.getCachedFilePath(key);
return cachePath;
}
{code}
# Below type casting can be replaced with HDFS-14393 interface.
{code:java}
/**
* Get cache capacity of persistent memory.
* TODO: advertise this metric to NameNode by FSDatasetMBean
*/
public long getPmemCacheCapacity() {
if (isPmemCacheEnabled()) {
return ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager().getPmemCacheCapacity();
}
return 0;
}
public long getPmemCacheUsed() {
if (isPmemCacheEnabled()) {
return ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager().getPmemCacheUsed();
}
return 0;
}
{code}
# {{FsDatasetUtil#deleteMappedFile}} - try-catch is not required, can we do
like,
{code:java}
public static void deleteMappedFile(String filePath) throws IOException {
....
....
boolean result = Files.deleteIfExists(Paths.get(filePath));
if (!result) {
throw new IOException("Failed to delete the mapped file: " + filePath);
}
}
{code}
# Why cant't we avoid {{LocalReplica}} changes and read directly from Util like
below,
{code:java}
FsDatasetImpl#getBlockInputStreamWithCheckingPmemCache()
.....
if (cachePath != null) {
return FsDatasetUtil.getInputStreamAndSeek(new File(cachePath),
seekOffset);
}
{code}
# As the class {{PmemVolumeManager}} itself represents {{Pmem}} so its good to
remove this extra keyword from the methods and entities from this class -
PmemUsedBytesCount, getPmemCacheUsed, getPmemCacheCapacity etc..
# Please avoid unchecked conversion and we can do like,
{code:java}
PmemVolumeManager.java
private final Map<ExtendedBlockId, Byte> blockKeyToVolume =
new ConcurrentHashMap<>();
Map<ExtendedBlockId, Byte> getBlockKeyToVolume() {
return blockKeyToVolume;
}
{code}
# Add exception message in PmemVolumeManager#verifyIfValidPmemVolume
{code:java}
if (out == null) {
throw new IOException();
}
{code}
# Here {{IOException}} clause is not required, please remove it. We can add it
later, if needed.
{code:java}
MappableBlock.java
void afterCache() throws IOException;
FsDatasetCache.java
try {
mappableBlock.afterCache();
} catch (IOException e) {
LOG.warn(e.getMessage());
return;
}
{code}
# Can we include block id into the log message, that would improve debugging.
{code:java}
LOG.info("Successfully cache one replica into persistent memory: " +
"[path=" + filePath + ", length=" + length + "]");
to
LOG.info("Successfully cached one replica:{} into persistent memory"
+ ", [cached path={}, length={}]", key, filePath, length);
{code}
was (Author: rakeshr):
Adding review comments, please take care.
# How about adding an API to interface
{{MappableBlockLoader#isTransientCache()}} to avoid checks specific to PMem. It
can return specific flag value to differentiate NVMe/DRAM based cache.
{code:java}
public boolean isPmemCacheEnabled() {
return mappableBlockLoader instanceof PmemMappableBlockLoader;
}
{code}
# I'd like to avoid type casting. It won't work with another Pmem
implementation, right?
{code:java}
public String getReplicaCachePath(String bpid, long blockId) {
if (!isPmemCacheEnabled() || !isCached(bpid, blockId)) {
return null;
}
ExtendedBlockId key = new ExtendedBlockId(blockId, bpid);
String cachePath = ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager()
.getCachedFilePath(key);
return cachePath;
}
{code}
# Below type casting can be replaced with HDFS-14393 interface.
{code:java}
/**
* Get cache capacity of persistent memory.
* TODO: advertise this metric to NameNode by FSDatasetMBean
*/
public long getPmemCacheCapacity() {
if (isPmemCacheEnabled()) {
return ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager().getPmemCacheCapacity();
}
return 0;
}
public long getPmemCacheUsed() {
if (isPmemCacheEnabled()) {
return ((PmemMappableBlockLoader)mappableBlockLoader)
.getPmemVolumeManager().getPmemCacheUsed();
}
return 0;
}
{code}
# {{FsDatasetUtil#deleteMappedFile}} - try-catch is not required, can we do
like,
{code:java}
public static void deleteMappedFile(String filePath) throws IOException {
....
....
boolean result = Files.deleteIfExists(Paths.get(filePath));
if (!result) {
throw new IOException("Failed to delete the mapped file: " + filePath);
}
}
{code}
# Why cant't we avoid {{LocalReplica}} changes and read directly from Util
like below,
{code:java}
FsDatasetImpl#getBlockInputStreamWithCheckingPmemCache()
.....
if (cachePath != null) {
return FsDatasetUtil.getInputStreamAndSeek(new File(cachePath),
seekOffset);
}
{code}
# As the class {{PmemVolumeManager}} itself represents {{Pmem}} so its good to
remove this extra keyword from the methods and entities from this class -
PmemUsedBytesCount, getPmemCacheUsed, getPmemCacheCapacity etc..
# Please avoid unchecked conversion and we can do like,
{code:java}
PmemVolumeManager.java
private final Map<ExtendedBlockId, Byte> blockKeyToVolume =
new ConcurrentHashMap<>();
Map<ExtendedBlockId, Byte> getBlockKeyToVolume() {
return blockKeyToVolume;
}
{code}
# Add exception message in PmemVolumeManager#verifyIfValidPmemVolume
{code:java}
if (out == null) {
throw new IOException();
}
{code}
# Here {{IOException}} clause is not required, please remove it. We can add it
later, if needed.
{code:java}
MappableBlock.java
void afterCache() throws IOException;
FsDatasetCache.java
try {
mappableBlock.afterCache();
} catch (IOException e) {
LOG.warn(e.getMessage());
return;
}
{code}
# Can we include block id into the log message, that would improve debugging.
{code:java}
LOG.info("Successfully cache one replica into persistent memory: " +
"[path=" + filePath + ", length=" + length + "]");
to
LOG.info("Successfully cached one replica:{} into persistent memory"
+ ", [cached path={}, length={}]", key, filePath, length);
{code}
> 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
>
>
> 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: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
