[
https://issues.apache.org/jira/browse/HDFS-4353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13543397#comment-13543397
]
Todd Lipcon commented on HDFS-4353:
-----------------------------------
{code}
- sock.setSoTimeout(dfsClient.getConf().socketTimeout);
{code}
did this timeout setting (just after the newPeer call) get lost in the refactor?
----
{code}
+ private Peer peer;
+ private long time;
{code}
Make these final
----
{code}
+ private Daemon daemon;
{code}
Put with the other variable declarations (below {{class Value}}
----
- Rename {{scInstance}} to {{pcInstance}} or just {{instance}}
- Javadoc for {{PeerCache.get(DataNode)}} is out of date - still refers to old
params and types
----
{code}
+ public synchronized void put(DatanodeID dnId,
org.apache.hadoop.hdfs.net.Peer peer) {
{code}
Shouldn't need fully qualified class name here
----
{code}
- this.socketIn = NetUtils.getInputStream(s);
- this.socketOut = NetUtils.getOutputStream(s, dnConf.socketWriteTimeout);
- this.isLocal = s.getInetAddress().equals(s.getLocalAddress());
+ this.socketIn = peer.getInputStream();
+ this.socketOut = peer.getOutputStream();
{code}
{{peer.getOutputStream}} here calls {{new SocketOutputStream(channel, 0)}} --
ie doesn't set a write timeout. However, the previous code here did set a
write timeout. Does this turn out to not matter due to another call later?
I have the same question in a number of other places where we used to pass a
WRITE_TIMEOUT to {{NetUtils.getOutputStream}} but now appear to end up with
timeout 0 (eg in {{newBlockReader}}). I see a few explicit calls to
{{setWriteTimeout}} but can you please double check that we didn't miss any
cases? Our test coverage of timeout behavior is light.
----
{code}
+ * Free the resources associated with this peer factory.
+ * This normally includes sockets, etc.
+ *
+ * @throws IOException If there is an error closing the PeerFactory
{code}
Refers to "factory" when it should say "server".
----
The {{peerServer}} JavaDoc should explain a little bit more about what kind of
'tracking' it does of connections. I find it a little incongruous that
{{PeerServer}} is responsible for tracking the set of connected peers. Since
all connected clients have the same {{Peer}} interface, couldn't it stay as
part of DataXceiverServer (ie just rename {{childSockets}} to {{childPeers}} or
{{connectedPeers}}? Also seems strange to me that the {{PeerServer}} interface
would have {{removePeer}} but not {{addPeer}}.
Along the same lines, it seems odd that {{Peer.close}} would be responsible for
removing itself from the {{peerServer}} - it's kind of inverted responsibility
here, and odd since Peers are also used on clients with no associated server.
----
-{{TcpPeerServer}} missing audience annotation
> Encapsulate connections to peers in Peer and PeerServer classes
> ---------------------------------------------------------------
>
> Key: HDFS-4353
> URL: https://issues.apache.org/jira/browse/HDFS-4353
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, hdfs-client
> Affects Versions: 2.0.3-alpha
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: 02-cumulative.patch
>
>
> Encapsulate connections to peers into the {{Peer}} and {{PeerServer}}
> classes. Since many Java classes may be involved with these connections, it
> makes sense to create a container for them. For example, a connection to a
> peer may have an input stream, output stream, readablebytechannel, encrypted
> output stream, and encrypted input stream associated with it.
> This makes us less dependent on the {{NetUtils}} methods which use
> {{instanceof}} to manipulate socket and stream states based on the runtime
> type. it also paves the way to introduce UNIX domain sockets which don't
> inherit from {{java.net.Socket}}.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
