[
https://issues.apache.org/jira/browse/HDFS-5746?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Colin Patrick McCabe updated HDFS-5746:
---------------------------------------
Attachment: HDFS-5746.002.patch
bq. I didn't see anything named ShortCircuitSharedMemorySegment in the patch,
should it be included?
It should be there...
bq. Javadoc for SharedFileDescriptorFactory constructor
added
bq. rand() isn't reentrant, potentially making it unsafe for createDescriptor0.
Should we use rand_r instead, or slap a synchronized on it?
Apparently, on Linux rand is re-entrant because glibc puts a mutex around it.
But you're right, we should be POSIX-compliant here. I added a mutex around
rand. Using the reentrant versions would be awkward because of the need to
pass around state somehow (probably a java array).
bq. Also not sure why we concat two rand(). Seems like one should be enough
with the collision detection code.
fair enough.
bq. 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.
Good point. we don't want random users to be able to open this file during the
brief period it exists in the namespace.
bq. Paranoia, should we do a check in CloseableReferenceCount#reference for
overflow to the closed bit? I know we have 30 bits, but who knows.
Well, this code was just moved from DomainSocket.java, not changed.
The issue is that we want to use atomic addition, not compare-and-exchange, for
speed. Given that, all we know is the state after the addition, not before.
This is fairly performance-critical for UNIX domain sockets (it has to do this
before every socket operation) so it has to be fast. The failure mode also
seems fairly benign: the refcount overflows into the closed bit and causes the
socket to appear closed.
At some point we should evaulate a 64-bit counter. It might be just as fast on
64-bit machines.
bq. Unrelated nit: DomainSocket#write(byte[], int, int) boolean exec is
indented wrong, mind fixing it?
ok
bq. \[DomainSocketWatcher\] 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.
added
bq. 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.
It's kind of annoying that using an inner Runnable class would increase the
indentation of run(). Still, I suppose it does provide better isolation,
making it impossible to invoke random Thread methods on the
DomainSocketWatcher. So I will implement that.
bq. 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.
It's arbitrary: both sockets are connected to one another and exactly alike. I
chose to listen on 1 and write on 0, but I could easily have made the opposite
choice.
bq. "loopSocks" is also not a very descriptive name, maybe "wakeupPair" or
"eventPair" instead?
I changed it to {{notificationSockets}}.
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.
bq. 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.
Good catch. Fixed.
bq. closeAll parameter in sendCallback is unused
removed
bq. This comment probably means to refer to loopSocks: // Close
shutdownSocketPair\[0\], so that shutdownSocketPair\[1\] gets an EOF
ok
bq. This comment probably meant poll, not select: // were waiting in select().
ok
bq. Why are two of the @Test in TestDomainSocketWatcher commented out?
fixed
bq. Timeouts seem kind of long, these should be super fast tests right?
reduced. I didn't want to reduce too much to avoid flakiness.
> 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, HDFS-5746.002.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)
