hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-5746) add ShortCircuitSharedMemorySegment
Date Tue, 28 Jan 2014 03:11:38 GMT

    [ 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?

* 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?

* 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
* "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:
    // Close shutdownSocketPair[0], so that shutdownSocketPair[1] gets an EOF
* This comment probably meant poll, not select:
          // were waiting in select().

* 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

View raw message