[
https://issues.apache.org/jira/browse/HDFS-5746?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13883702#comment-13883702
]
Andrew Wang commented on HDFS-5746:
-----------------------------------
Nice work here. I have a fair number of review comments, but most of it's nitty:
I didn't see anything named ShortCircuitSharedMemorySegment in the patch,
should it be included?
SharedFileDescriptorFactory:
* Javadoc for SharedFileDescriptorFactory constructor
* {{rand()}} isn't reentrant, potentially making it unsafe for
{{createDescriptor0}}. Should we use {{rand_r}} instead, or slap a synchronized
on it?
* Also not sure why we concat two {{rand()}}. Seems like one should be enough
with the collision detection code.
* The {{open}} is done with mode {{0777}}, wouldn't {{0700}} be safer? I
thought we were passing these over a domain socket, so we can keep the
permissions locked up.
* Paranoia, should we do a check in CloseableReferenceCount#reference for
overflow to the closed bit? I know we have 30 bits, but who knows.
* Unrelated nit: DomainSocket#write(byte[], int, int) {{boolean exec}} is
indented wrong, mind fixing it?
DomainSocketWatcher:
* Class javadoc is c+p from {{DomainSocket}}, I think it should be updated for
DSW. Some high-level description of how the nested classes fit together would
be nice.
* Some Java-isms. {{Runnable}} is preferred over {{Thread}}. It's also weird
that DSW is a {{Thread}} subclass and it calls {{start}} on itself. An inner
class implementing Runnable would be more idiomatic.
* Explain use of {{loopSocks 0}} versus {{loopSocks 1}}? This is a crucial part
of this class: we need to use a socketpair rather than a plain condition
variable because of blocking on poll.
* "loopSocks" is also not a very descriptive name, maybe "wakeupPair" or
"eventPair" instead?
* Can add a Precondition check to make sure the lock is held in checkNotClosed
* If we fail to kick, add and remove could block until the poll timeout.
* Should doc that we only support one Handler per fd, it overwrites on add.
Maybe Precondition this instead if we don't want to overwrite, I can't tell
from context here.
* Typo "loopSOcks" in log message
* The repeated calls to {{sendCallback}} are worrisome. For instance, a sock
could be EOF and closed, be removed by the first sendCallback, and then if
there's a pending toRemove for the sock, the second sendCallback aborts on the
Precondition check.
* {{closeAll}} parameter in sendCallback is unused
* This comment probably means to refer to loopSocks:
{code}
// Close shutdownSocketPair[0], so that shutdownSocketPair[1] gets an EOF
{code}
* This comment probably meant poll, not select:
{code}
// were waiting in select().
{code}
TestDomainSocketWatcher:
* Why are two of the {{@Test}} in TestDomainSocketWatcher commented out?
* Timeouts seem kind of long, these should be super fast tests right?
> add ShortCircuitSharedMemorySegment
> -----------------------------------
>
> Key: HDFS-5746
> URL: https://issues.apache.org/jira/browse/HDFS-5746
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, hdfs-client
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Fix For: 3.0.0
>
> Attachments: HDFS-5746.001.patch
>
>
> Add ShortCircuitSharedMemorySegment, which will be used to communicate
> information between the datanode and the client about whether a replica is
> mlocked.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
