Ozone:SCM: Add support for registerNode in datanode by anuengineer · Pull Request #151 · apache/hadoop

anuengineer · 2016-11-03T22:59:56Z

No description provided.

xiaoyuyao · 2016-11-13T04:05:21Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneClientUtils.java

+  /**
+   * Returns a hostname from the hostname:port or hostname.
+   * @param value
+   * @return


Javadoc for the pos parameter is missing. Suggest document pos (0/1) usage or use a Enum like HostComponentType for safer usage as we don't have the validation of pos inside getHostComponent().
pos(0) -> host
pos(1) -> port

fixed. Thanks for the comment. I have replaced the parsing code with HostAndPort class. So this code has becomes simpler and I have updated the Javadocs. So we don't need the pos parameter any longer.

xiaoyuyao · 2016-11-13T04:07:30Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneClientUtils.java

+  }
+
+  public static Optional<String> getHostName(String value) {
+    final int hostIndex = 0;


This can be a Enum as commented above.

Replaced this logic with HostAndPort class.

xiaoyuyao · 2016-11-13T04:12:45Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneClientUtils.java

-        }
-      }
+      return getHostPort(value);
    }


How do we handle multiple keys but only the last key has a valid value? The new logic will return Optional.absent() in the first loop instead of the last valid value.

Thanx fixed. The intention is to return the first key with valid hostport, now it checks for the isPresent and returns the value if it is present or loops till the end of the key space.

xiaoyuyao · 2016-11-13T04:13:17Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneClientUtils.java

   */
-  static Optional<Integer> getPortNumberFromConfigKeys(
+  public static Optional<Integer> getPortNumberFromConfigKeys(
      Configuration conf, String ... keys) {


Can we add complete java docs for the parameters after make this public?

xiaoyuyao · 2016-11-13T04:19:06Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneClientUtils.java

+   */
+  public static int getScmRpcTimeOutInMilliseconds(Configuration conf) {
+    return conf.getInt(OZONE_SCM_HEARTBEAT_RPC_TIMEOUT_MS,
+        OZONE_SCM_HEARTBEAT_RPC_TIMEOUT_MS_DEFAULT);


I would suggest we use Configuration#getTimeDuration() instead of Configuration#getInt() for durations. This will reduce the documentation cost and chance of mis-configuration in deployment. An sample usage can be found in datanode for DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY.

xiaoyuyao · 2016-11-13T04:23:12Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

+      Runtime.getRuntime().availableProcessors() * 2;
+
+  public static final String OZONE_SCM_HEARTBEAT_RPC_TIMEOUT_MS =
+      "ozone.scm.heartbeat.rpc-timeout.ms";


Suggest using time duration property with suffix and remove the .ms from the configuration key.

xiaoyuyao · 2016-11-13T04:24:50Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

+      10;
+
+  public static final String OZONE_CONTAINER_TASK_WAIT =
+      "ozone.container.task.wait.seconds";


Suggest using time duration property with suffix and remove the .seconds from the configuration key.

This key has been removed.

You mean remove the .seconds from the key name.

xiaoyuyao · 2016-11-13T04:26:21Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

+
+
+  // ozone.scm.names key is a set of DNS | DNS:PORT | IP Address | IP:PORT.
+  // Written as a comma separated string. e.g. scm1, scm2:8020, 7.7.7.7:7777


Do we have unit test for ozone.scm.names like "scm1, scm2:8020, 7.7.7.7:7777"

I will post another patch this negative test cases . I have filed HDFS-11137 to track this issue.

xiaoyuyao · 2016-11-13T04:28:07Z

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

+  public static final String OZONE_SCM_NAMES = "ozone.scm.names";
+
+  public static final int OZONE_SCM_DEFAULT_PORT = 9862;
+  // Full path the location where datanode ID is to written to.


NIT: Maybe we should say "file name" instead of "Full path" here.

xiaoyuyao · 2016-11-13T04:30:05Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeStateMachine.java

+        new ThreadFactoryBuilder().setDaemon(true)
+            .setNameFormat("Container State Machine Thread - %d").build());
+    connectionManager = new SCMConnectionManager(
+        OzoneClientUtils.getScmRpcTimeOutInMilliseconds(conf),


OzoneClientUtils.getScmRpcTimeOutInMilliseconds(conf) can be encapsulated inside SCMConnectionManager constructor as we have passed the conf parameter anyway.

xiaoyuyao · 2016-11-13T04:38:12Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeStateMachine.java

+      }
+    } catch (InterruptedException e) {
+      executorService.shutdownNow();
+      Thread.currentThread().interrupt();


Can we add a Log.error() after the Thread.currentThread.interrupt() for troubleshooting of shutdown issues in the future?

xiaoyuyao · 2016-11-13T04:47:46Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/EndpointStateMachine.java

+  static final Logger
+      LOG = LoggerFactory.getLogger(EndpointStateMachine.class);
+  private final StorageContainerDatanodeProtocolClientSideTranslatorPB endPoint;
+  private final AtomicInteger missedCount;


AtomicLong instead of AtomicInteger?

xiaoyuyao · 2016-11-13T04:53:18Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/EndpointStateMachine.java

+    }
+
+    /**
+     * Returns the next logical state that endPoint should move to.


Can you document the same assumption that the state is transited by value + 1.

xiaoyuyao · 2016-11-13T04:54:34Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/SCMConnectionManager.java

+  private final Configuration conf;
+
+
+  public SCMConnectionManager(int rpcTimeout, Configuration conf) {


rpcTimeout parameter can be removed

xiaoyuyao · 2016-11-13T04:58:07Z

...rc/main/java/org/apache/hadoop/ozone/container/common/statemachine/SCMConnectionManager.java

+      EndpointStateMachine endPoint =
+          new EndpointStateMachine(address, rpcClient, conf);
+      scmMachines.put(address, endPoint);
+


NIT: extra blank line.

xiaoyuyao · 2016-11-13T16:10:04Z

...p-hdfs/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java

+   * if you need to execute a task without any concurrent execution, please
+   * return a single task in this list.
+   *
+   * @return List of Callables


I don't think we return a List here. Can you clarify here as well as "Returns a lit of tasks that..;"

xiaoyuyao · 2016-11-13T16:13:18Z

...p-hdfs/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/package-info.java

+
+ Container has the following states.
+
+ Start - > getVersion -> Register -> Running +-> Upgrade -> Shutdown


Should we update the states document here without upgrade/decommission state since they are not implemented yet.

xiaoyuyao · 2016-11-13T16:15:57Z

...rc/main/java/org/apache/hadoop/ozone/container/common/states/datanode/InitDatanodeState.java

+import java.util.concurrent.TimeoutException;
+
+/**
+ * Init Container Task is the task that gets run when we are in Init State.


javadoc needs update? init container task-> init datanode state

xiaoyuyao · 2016-11-14T15:52:12Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+          OzoneConfigKeys.OZONE_SCM_DATANODE_ID);
+
+      // This is an unrecoverable error.
+      this.context.setState(DatanodeStateMachine.DatanodeStates.SHUTDOWN);


Should we return null after mark DatanodeStates.SHUTDOWN instead of continue attempt to read the container ID?

xiaoyuyao · 2016-11-14T15:53:20Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+    try {
+      nodeID = readPersistedDatanodeID(Paths.get(dataNodeIDPath));
+      if (nodeID != null) {
+        return nodeID;


Add a LOG.trace() for containerID read from file successfully.

xiaoyuyao · 2016-11-14T15:54:12Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+    // info to SCM.
+    try {
+      nodeID = createNewContainerID(Paths.get(dataNodeIDPath));
+      return nodeID;


Add a trace for ContainerID created?

xiaoyuyao · 2016-11-14T16:20:03Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+   * Computes the next state the container state machine must move to by looking
+   * at all the state of endpoints.
+   * <p>
+   * if any endpoint state has moved to Register state, then the Container State


"if any endpoint state has moved to Register state,...", this seems not being enforced in the code below.

xiaoyuyao · 2016-11-14T16:28:10Z

...main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java

+      return new VersionEndpointTask(endpoint, conf);
+    case REGISTER:
+      RegisterEndpointTask task = new RegisterEndpointTask(endpoint, conf);
+      task.setContainerNodeIDProto(getContainerNodeID());


NIT: maybe use a builder pattern to avoid creating RegisterEndpointTask without ContainerNodeID.

xiaoyuyao · 2016-11-14T16:28:45Z

...ain/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java

+   *
+   * @return ContainerNodeIDProto
+   */
+  public ContainerNodeIDProto getContainerNodeIDProto() {


Builder pattern as suggested earlier.

xiaoyuyao · 2016-11-14T16:30:48Z

...main/java/org/apache/hadoop/ozone/container/common/states/endpoint/RegisterEndpointTask.java

+   * @return ContainerNodeIDProto
+   */
+  public ContainerNodeIDProto getContainerNodeIDProto() {
+    return containerNodeIDProto;


Builder pattern.

xiaoyuyao · 2016-11-14T16:33:36Z

...main/java/org/apache/hadoop/ozone/container/common/states/endpoint/RegisterEndpointTask.java

+    if (getContainerNodeIDProto() == null) {
+      LOG.error("Container ID proto cannot be null in RegisterEndpoint task, " +
+          "shutting down the endpoint.");
+      rpcEndPoint.setState(EndpointStateMachine.EndPointStates.SHUTDOWN);


NIT: Consider returning the new state from rpcEndPoint.setState so that we can consolidate logic here?

xiaoyuyao · 2016-11-14T16:43:19Z

...ain/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java

+      rpcEndpoint.zeroMissedCount();
+    } catch (IOException ex) {
+      rpcEndpoint.logIfNeeded(ex,
+          OzoneClientUtils.getScmHeartbeatInterval(this.conf));


The second parameter can be removed as ScmHeartbeatInterval is available within rpcEndpoint.logIfNeeded() with RpcEndpoint#conf.

xiaoyuyao · 2016-11-14T16:55:25Z

...dfs/src/main/java/org/apache/hadoop/ozone/protocolPB/StorageContainerDatanodeProtocolPB.java

+import org.apache.hadoop.ozone.protocol.proto.StorageContainerDatanodeProtocolProtos.StorageContainerDatanodeProtocolService;
+
+/**
+ * Protocol used from an HDFS node to StorageContainerManager.  This extends the


Can you elaborate the comment on "HDFS node"?

xiaoyuyao · 2016-11-14T17:18:52Z

...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/ozone/scm/node/SCMNodeManager.java

-        datanodeID);
-    nodes.put(newDatanodeID.getDatanodeUuid(), newDatanodeID);
+
+    nodes.put(datanodeID.getDatanodeUuid(), datanodeID);


How do we handle the case when two datanodes attempt to register with the same datanodeID? I don't think we should allow both succeed without warning or error.

Ozone:SCM: Add support for registerNode in datanode

ef7262f

xiaoyuyao reviewed Nov 13, 2016

View reviewed changes

xiaoyuyao reviewed Nov 14, 2016

View reviewed changes

hadoop-yetus mentioned this pull request Dec 7, 2021

HADOOP-18036 : Backport HADOOP-17653 for branch-3.2 #3758

Closed

This was referenced Feb 19, 2026

YARN-11897: Make NMWebservices API backward-compatible after Jersey 2… #8227

Open

YARN-11937. Forward hadoop-jwt with YARN proxy #8273

Closed

hadoop-yetus mentioned this pull request Feb 26, 2026

Bump rollup from 4.57.1 to 4.59.0 in /hadoop-yarn-project/hadoop-yarn/hadoop-yarn-capacity-scheduler-ui/src/main/webapp #8280

Open



		// ozone.scm.names key is a set of DNS \| DNS:PORT \| IP Address \| IP:PORT.
		// Written as a comma separated string. e.g. scm1, scm2:8020, 7.7.7.7:7777

		private final Configuration conf;


		public SCMConnectionManager(int rpcTimeout, Configuration conf) {


		Container has the following states.

		Start - > getVersion -> Register -> Running +-> Upgrade -> Shutdown

Conversation

anuengineer commented Nov 3, 2016

Uh oh!

xiaoyuyao Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xiaoyuyao Nov 13, 2016 •

edited

Loading