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

stack commented on HDFS-7844:
-----------------------------

Late to the game.

Patch is great. Creative.

All comments can be addressed later.

High level, any notion of difference in perf when comparing native to offheap 
to current implementation?

If we fail to pick up the configured memory manager (or the default), its worth 
a WARN log. Otherwise, folks may be confounded that they are getting the native 
memory manager though they asked for something else:

83          public static MemoryManager create(String name, Configuration conf) 
{
84            String memoryManagerKey = conf.get(
85                CommonConfigurationKeys.HADOOP_MEMORY_MANAGER_KEY,
86                CommonConfigurationKeys.HADOOP_MEMORY_MANAGER_DEFAULT);
87            if (memoryManagerKey == null) {
88              memoryManagerKey = NativeMemoryManager.class.getCanonicalName();
89            }

This an arbitrary max?

 private final static long MAX_ADDRESS = 0x3fffffffffffffffL;

ByteArrayMemoryManager is just throwaway, testing? Otherwise, protect Log.TRACE 
with LOG.isTraceEnabled...

Its fun the way you did ByteArrayMemoryManager mapping address to a Map.

Ok, I see, BAMM is just for testing. Ignore above.

nit: make a method rather than dup the below...:

145         Entry<Long, byte[]> entry = buffers.floorEntry(Long.valueOf(addr));
146         if (entry == null) {
147           throw new RuntimeException("Wrote to unallocated address 0x" +
148               Long.toHexString(addr));
149         }

method would return a byte array gotten from TreeMap... etc.

Is logging open at DEBUG but close at TRACE lead to confusion? Stumped debugger?

The lose has to let out an IOE? What is the caller going to do w/ this IOE?  
The ByteArrayMemoryManager close error string construction is same as close on 
ProbingHashTable?

Yeah man, put the Log.TRACE behind a test for TRACEyness.

I buy the compactness invariant.

I like the compromise put upon the Iterator (that resize is allowed while 
Iteration...) Seems appropriate given where this is to be deployed.

On TestMemoryManager, maybe parameterize so once through with 
ByteArrayMemoryManager and then a run with the offheap implementation rather 
than have dedicated test for each: 
https://github.com/junit-team/junit/wiki/Parameterized-tests

TestProbingHashTable is fun with its BlockInfo, etc., implementations.



















> Create an off-heap hash table implementation
> --------------------------------------------
>
>                 Key: HDFS-7844
>                 URL: https://issues.apache.org/jira/browse/HDFS-7844
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>    Affects Versions: HDFS-7836
>            Reporter: Colin Patrick McCabe
>            Assignee: Colin Patrick McCabe
>         Attachments: HDFS-7844-scl.001.patch, HDFS-7844-scl.002.patch, 
> HDFS-7844-scl.003.patch
>
>
> Create an off-heap hash table implementation.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to