adoroszlai commented on code in PR #9997:
URL: https://github.com/apache/ozone/pull/9997#discussion_r3005851164
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java:
##########
@@ -161,49 +159,55 @@ public void connect() throws Exception {
connectToDatanode(dn);
}
- private void connectToDatanode(DatanodeDetails dn)
- throws IOException {
+ private void connectToDatanode(DatanodeDetails dn) throws IOException {
if (isClosed.get()) {
throw new IOException("Client is closed.");
}
if (isConnected(dn)) {
return;
}
- // read port from the data node, on failure use default configured port
- int port = dn.getStandalonePort().getValue();
- if (port == 0) {
- port = config.getInt(OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT,
- OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT_DEFAULT);
- }
- final int finalPort = port;
- LOG.debug("Connecting to server : {}; nodes in pipeline : {}, ", dn,
pipeline.getNodes());
+ LOG.debug("Connecting to server: {}; nodes in pipeline: {}", dn,
pipeline.getNodes());
- channels.computeIfPresent(dn.getID(), (dnId, channel) -> {
- if (channel.isTerminated() || channel.isShutdown()) {
- asyncStubs.remove(dnId);
- return null; // removes from channels map
- }
+ removeStaleChannel(dn);
+ generateNewChannel(dn);
+ }
- return channel;
- });
+ /**
+ * Checks if the client has a live connection channel to the specified
Datanode.
+ *
+ * @return True if the connection is alive, false otherwise.
+ */
+ @VisibleForTesting
+ public boolean isConnected(DatanodeDetails details) {
Review Comment:
nit: please don't move `isConnected` unnecessarily, it inflates the diff
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java:
##########
@@ -161,49 +159,55 @@ public void connect() throws Exception {
connectToDatanode(dn);
}
- private void connectToDatanode(DatanodeDetails dn)
- throws IOException {
+ private void connectToDatanode(DatanodeDetails dn) throws IOException {
if (isClosed.get()) {
throw new IOException("Client is closed.");
}
if (isConnected(dn)) {
return;
}
- // read port from the data node, on failure use default configured port
- int port = dn.getStandalonePort().getValue();
- if (port == 0) {
- port = config.getInt(OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT,
- OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT_DEFAULT);
- }
- final int finalPort = port;
- LOG.debug("Connecting to server : {}; nodes in pipeline : {}, ", dn,
pipeline.getNodes());
+ LOG.debug("Connecting to server: {}; nodes in pipeline: {}", dn,
pipeline.getNodes());
- channels.computeIfPresent(dn.getID(), (dnId, channel) -> {
- if (channel.isTerminated() || channel.isShutdown()) {
- asyncStubs.remove(dnId);
- return null; // removes from channels map
- }
+ removeStaleChannel(dn);
+ generateNewChannel(dn);
+ }
Review Comment:
`connectToDatanode` still performs multiple map operations in a non-atomic
way:
- calls `isConnected` twice, once directly, once via `removeStaleChannel`
- `isConnected` calls `containsKey` and `get` separately
- `removeStaleChannel` calls `isConnected` and `remove` separately
- `generateNewChannel` calls unconditional `put`
Use only one `compute` operation, and distinguish between the three cases in
that call:
- absent
- present but stale
- present and active
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java:
##########
@@ -784,4 +772,25 @@ public ConfigurationSource getConfig() {
public void setTimeout(long timeout) {
this.timeout = timeout;
}
+
+ /**
+ * Group the channel and stub so that they are published together.
+ */
+ private static class ChannelInfo {
+ private ManagedChannel channel;
+ private XceiverClientProtocolServiceStub stub;
Review Comment:
nit: these can be `final`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
