[
https://issues.apache.org/jira/browse/HDFS-7844?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14351073#comment-14351073
]
Charles Lamb commented on HDFS-7844:
------------------------------------
[~cmccabe],
This is a nice piece of work!
Here are some comments:
General:
Several lines bust the 80 char limit.
Many unused imports throughout. I guess Yi got this already.
What happens if someone runs this with the -d32 to the jvm? Do we need to make
that check and throw accordingly?
ProbingHashSet.java:
A small enhancement might be: {code}close(boolean force){code} which will close
unconditionally.
The line in #getSlot which is {code}hash = -hash{code} is in fact tested by
your unit tests, but I don't think it's tested by design in the test. You might
want to put in an explicit test for that particular line.
#expandTable: using {code}catch(Throwable){code} feels like a rather wide net
to cast, but I guess it's the right thing. I debated whether all you needed was
catch (Error), but I guess you can't be sure that the callers above you won't
just "keep going" after some RuntimeException gets into their hands.
The comment for #capacity() "total number of slots" is either misleading or
wrong.
MemoryManager.java
any reason not to have get/putShort along with the existing byte/int/long?
Should #toString() be declared as {code}@Override public String toString(){code}
NativeMemoryManager.java
The comments say nothing about whether it's thread safe or not. Ditto for
ByteArrayMemoryManager.
ByteArrayMemoryManager
There is no test coverage for the failure case of {code}BAMM.close(){code}
s/valiation/validation/ (Yi caught this)
Why does curAddress start at 1000?
s/2^^31/2^31/
For all of the put/get/byte/int/long routines, it wouldn't be hard to move all
of the {code}if() { throw new RuntimeException }{code} snippits into their own
routine. Maybe that's not worth the trouble, but if feels like there's a lot of
repeated code.
TestMemoryManager.java
The indentation of #testMemoryManagerCreate formals is messed up.
#testCatchInvalidPuts: you test putByte against freed memory, but not int or
long.
the Assert.fail messages should be different for each fail() call.
The exception checks in getByte/Int/Long are not tested.
None of the entry==null exceptions are tested in putByte/Long/Int
I tried running TestMemoryManager.testNativeMemoryManagerCreate and it failed
like this:
{code}
2015-03-06 17:10:22,430 ERROR offheap.MemoryManager$Factory
(MemoryManager.java:create(91)) - Unable to create
org.apache.hadoop.util.offheap.NativeMemoryManager. Falling back on
org.apache.hadoop.util.offheap.ByteArrayMemoryManager
java.lang.IllegalArgumentException: wrong number of arguments
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at
sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
at
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
at
org.apache.hadoop.util.offheap.MemoryManager$Factory.create(MemoryManager.java:89)
at
org.apache.hadoop.util.offheap.TestMemoryManager.testMemoryManagerCreate(TestMemoryManager.java:135)
at
org.apache.hadoop.util.offheap.TestMemoryManager.testNativeMemoryManagerCreate(TestMemoryManager.java:151)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:606)
at
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
at
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
at
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at
org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
org.junit.ComparisonFailure:
Expected :org.apache.hadoop.util.offheap.NativeMemoryManager
Actual :org.apache.hadoop.util.offheap.ByteArrayMemoryManager
<Click to see difference>
at org.junit.Assert.assertEquals(Assert.java:115)
at org.junit.Assert.assertEquals(Assert.java:144)
at
org.apache.hadoop.util.offheap.TestMemoryManager.testMemoryManagerCreate(TestMemoryManager.java:137)
at
org.apache.hadoop.util.offheap.TestMemoryManager.testNativeMemoryManagerCreate(TestMemoryManager.java:151)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
at
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
at
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at
org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
{code}
TestProbingHashSet.java:
Having Assert.assertFalse(hset.isEmpty()) in so many places alongside the
hset.size() feels like overkill, but I guess it's not hurting anyone.
> 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
>
>
> Create an off-heap hash table implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
