[
https://issues.apache.org/jira/browse/HDFS-7844?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14351122#comment-14351122
]
Colin Patrick McCabe commented on HDFS-7844:
--------------------------------------------
bq. Several lines bust the 80 char limit.
Thanks, I fixed a few cases.
bq. What happens if someone runs this with the -d32 to the jvm? Do we need to
make that check and throw accordingly?
I don't think this would be a problem. We don't care about big java references
are, since we're not using them. Even 32-bit machines should support
{{Unsafe#getLong}}-- if necessary, through two 32-bit operations.
bq. A small enhancement might be: close(boolean force) which will close
unconditionally.
I realize this is maybe a bit confusing, but we don't ever want to close with
entries remaining in the hash table. The reason is because the caller is
responsible for managing that memory. We wouldn't know what to do with it, so
there would be a memory leak.
Another thing to note is that in general, {{ProbingHashTable#close}} is really
only a unit test thing. In real life, we would never actually close the
BlocksMap in the NameNode... we'd just shut down the whole process when someone
control-Cs. So we don't have to worry about how long close takes :)
bq. The line in #getSlot which is hash = -hash 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.
Believe me, without that line, the unit tests don't work. Ask me how I know. :)
bq. expandTable: using catch(Throwable) 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.
It's a little bit of paranoia on my part. Really, there should be no
exceptions at all coming from that code, but given that this is Java, we can
never actually guarantee that. Even methods that aren't declared to throw a
particular exception can throw it, through the magic of classloaders. I wanted
to be able to guarantee that there were no memory leaks, and this was the only
way. And no, we can't rethrow the {{Throwable}} itself, because then Java
complains that the function isn't declared to throw {{Throwable}}. So we wrap
it in a {{RuntimeException}}.
bq. The comment for #capacity() "total number of slots" is either misleading or
wrong.
Thanks. Let's just replace this with an accessor for {{numSlots}}. Now that
the load factor is configurable, "capacity" is kind of a confusing term.
bq. any reason not to have get/putShort along with the existing byte/int/long?
Good idea, let's add that
bq. Should #toString() be declared as...
If you mean the toString function in the interfaces, everything in an interface
is always public. And putting @Override there is not needed. The other places
are already public and have @Override.
bq. [MemoryManager] comments say nothing about whether it's thread safe or
not. Ditto for ByteArrayMemoryManager.
Let me add a comment to the base class JavaDoc.
bq. There is no test coverage for the failure case of {{BAMM#close}}
added
bq. Why does curAddress start at 1000?
It can start at any address other than 0.
bq. For all of the put/get/byte/int/long routines, it wouldn't be hard to move
all of the if() { throw new RuntimeException } snippits into their own routine.
Maybe that's not worth the trouble, but if feels like there's a lot of repeated
code.
I think this is not worth the trouble. Maybe later.
bq. The indentation of #testMemoryManagerCreate formals is messed up.
ok
bq. testCatchInvalidPuts: you test putByte against freed memory, but not int or
long.
yeah let's test them all
bq. the Assert.fail messages should be different for each fail() call.
It's fine. There is a line number.
bq. I tried running TestMemoryManager.testNativeMemoryManagerCreate and it
failed like this:
This was bugged after I added the "name" argument to the MemoryManager. Should
be fixed now.
> 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)
