[
https://issues.apache.org/jira/browse/HDFS-941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12847117#action_12847117
]
Todd Lipcon commented on HDFS-941:
----------------------------------
Style notes:
- in BlockReader:
{code}
+ LOG.warn("Could not write to datanode " + sock.getInetAddress() +
+ ": " + e.getMessage());
{code}
should be more specific - like "Could not write read result status code" and
also indicate in the warning somehow that this is not a critical problem.
Perhaps "info" level is better? (in my experience if people see WARN they think
something is seriously wrong)
- please move the inner SocketCacheEntry class down lower in DFSInputStream
- in SocketCacheEntry.setOwner, can you use IOUtils.closeStream to close
reader? Similarly in SocketCacheEntry.close
- We expect the following may happen reasonably often, right?
{code}
+ // Our socket is no good.
+ DFSClient.LOG.warn("Error making BlockReader. Closing stale " +
entry.sock.toString());
{code}
I think this should probably be debug level.
- The edits to the docs in DataNode.java are good - if possible they should
probably move into HDFS-1001 though, no?
- the do { ... } while () loop is a bit hard to follow in DataXceiver. Would it
be possible to rearrange the code a bit to be more linear? (eg setting
DN_KEEPALIVE_TIMEOUT right before the read at the beginning of the loop if
workDone > 0 would be easier to follow in my opinion)
- In DataXceiver:
{code}
+ } catch (IOException ioe) {
+ LOG.error("Error reading client status response. Will close
connection. Err: " + ioe);
{code}
Doesn't this yield error messages on every incomplete client read? Since the
response is optional, this seems more like a DEBUG.
Bigger stuff:
- I think there is a concurrency issue here. Namely, the positional read API
calls through into fetchBlockByteRange, which will use the existing cached
socket, regardless of other concurrent operations. So we may end up with
multiple block readers on the same socket and everything will fall apart.
Can you add a test case which tests concurrent use of a DFSInputStream? Maybe a
few threads doing random positional reads while another thread does seeks and
sequential reads?
- Regarding the cache size of one - I don't think this is quite true. For a use
case like HBase, the region server is continually slamming the local datanode
with random read requests from several client threads. Is the idea that such an
application should be using multiple DFSInputStreams to read the same file and
handle the multithreading itself?
- In DataXceiver, SocketException is caught and ignored while sending a block.
("// Its ok for remote side to close the connection anytime." I think there are
other SocketException types (eg timeout) that could throw here aside from a
connection close, so in that case we need to IOUtils.closeStream(out) I
believe. A test case for this could be to open a BlockReader, read some bytes,
then stop reading so that the other side's BlockSender generates a timeout.
- Not sure about this removal in the finally clause of opWriteBlock:
{code}
- IOUtils.closeStream(replyOut);
{code}
(a) We still need to close in the case of an downstream-generated exception.
Otherwise we'll read the next data bytes from the writer as an operation and
have undefined results.
(b) To keep this patch less dangerous, maybe we should not add the reuse
feature for operations other than read? Read's the only operation where we
expect a lot of very short requests coming in - not much benefit for writes,
etc, plus they're more complicated.
> Datanode xceiver protocol should allow reuse of a connection
> ------------------------------------------------------------
>
> Key: HDFS-941
> URL: https://issues.apache.org/jira/browse/HDFS-941
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: data-node, hdfs client
> Affects Versions: 0.22.0
> Reporter: Todd Lipcon
> Assignee: bc Wong
> Attachments: HDFS-941-1.patch
>
>
> Right now each connection into the datanode xceiver only processes one
> operation.
> In the case that an operation leaves the stream in a well-defined state (eg a
> client reads to the end of a block successfully) the same connection could be
> reused for a second operation. This should improve random read performance
> significantly.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
