bharatviswa504 commented on a change in pull request #1494:
URL: https://github.com/apache/ozone/pull/1494#discussion_r543602650
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransport.java
##########
@@ -33,6 +35,9 @@
*/
OMResponse submitRequest(OMRequest payload) throws IOException;
+ BootstrapResponse bootstrapRequest(BootstrapRequest bootstrapRequest)
Review comment:
Minor: Add javadoc
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerRatisServer.java
##########
@@ -137,6 +141,59 @@ public OMResponse submitRequest(OMRequest omRequest)
throws ServiceException {
return processReply(omRequest, raftClientReply);
}
+ /**
+ * Add a new OM to the Ratis ring.
+ * @return true if the new OM is successfully added to the ratis ring,
+ * false otherwise.
+ */
+ public boolean bootstrapNewOMs(List<OMNodeDetails> newOMNodes) {
+ StringBuilder newOMNodeIdsBuilder = new StringBuilder();
+ newOMNodes.stream().forEach(newOMNode ->
+ newOMNodeIdsBuilder.append(", ").append(newOMNode.getOMNodeId()));
+ String newOMNodeIds = newOMNodeIdsBuilder.toString().substring(2);
+ LOG.info("Bootstrapping new OM(s): {}", newOMNodeIds);
Review comment:
We are not using httpAddrress, httpsAddress, rpcPort, serviceID which
are sent thru bootStraprequest
And in notifyConfigurationChanged finally reading from the current OM
config. So, I see there is no real use in sending these information only
required thing need looks like nodeID. Not sure If I am missing something here.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java
##########
@@ -388,6 +412,15 @@ public String
toStateMachineLogEntryString(StateMachineLogEntryProto proto) {
return OMRatisHelper.smProtoToString(proto);
}
+ @Override
+ public void close() throws IOException {
+ // OM should be shutdown as the StateMachine has shutdown.
Review comment:
Question:
I see this is called from Ratis, when any StateMachine has hit exception.
In this case, do we want to terminate OM, is this handled to handle any
failure of notifyConfigurationChanged where we are throwing
OzoneIllegalArgumentException?
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1203,11 +1203,41 @@ message UpdateGetS3SecretRequest {
required string awsSecret = 2;
}
+message OMNodeInfo {
+ required string nodeId = 1;
+ required string hostAddress = 2;
+ required uint32 rpcPort = 3;
Review comment:
We are not using rpcPort, httpAddr, httpsAddr, serviceId these can be
removed from this.
##########
File path:
hadoop-ozone/ozonefs-hadoop2/src/main/java/org/apache/hadoop/fs/ozone/Hadoop27RpcTransport.java
##########
@@ -71,6 +73,16 @@ public OMResponse submitRequest(OMRequest payload) throws
IOException {
}
}
+ @Override
+ public BootstrapResponse bootstrapRequest(BootstrapRequest bootstrapRequest)
+ throws IOException {
+ try {
+ return proxy.bootstrap(NULL_RPC_CONTROLLER, bootstrapRequest);
+ } catch (ServiceException e) {
+ throw new IOException("Service exception during the bootstrap", e);
Review comment:
In Hadoop3OMTransport, we have done some additional steps which are
missing here.
(And also right now Hadoop27OMTransport does not support OM HA, not related
to this, is that is the reason for not adding here, as failOverproxy is not
configured in Hadoop2 module?)
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1203,11 +1203,41 @@ message UpdateGetS3SecretRequest {
required string awsSecret = 2;
}
+message OMNodeInfo {
+ required string nodeId = 1;
+ required string hostAddress = 2;
+ required uint32 rpcPort = 3;
+ required uint32 ratisPort = 4;
+ required string httpAddr = 5;
+ required string httpsAddr = 6;
+ optional string serviceId = 7;
+}
+
+enum BootstrapErrorCode {
+ RATIS_NOT_ENABLED = 1;
+ LEADER_UNDETERMINED = 2;
+ RATIS_BOOTSTRAP_ERROR = 3;
+}
+
+message BootstrapRequest {
Review comment:
Can we call this AddOMRequest, as this is an admin command submitted to
add new OM's to the cluster.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerRatisServer.java
##########
@@ -317,8 +377,18 @@ public static OzoneManagerRatisServer newOMRatisServer(
.build();
List<RaftPeer> raftPeers = new ArrayList<>();
- // Add this Ratis server to the Ratis ring
- raftPeers.add(localRaftPeer);
+
+ // If the OM is started in bootstrap mode, do not add it to the ratis ring.
+ // It will be added later using SetConfiguration from the leader OM.
+ if (isBootstrapping) {
Review comment:
Just a question:
If OM is started normally, and prepareBootStrapRequest is performed will it
cause any issues?
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -192,6 +195,51 @@ public void close() throws IOException {
transport.close();
}
+
+ public List<ServiceInfo> getServiceList() throws IOException {
+ ServiceListRequest req = ServiceListRequest.newBuilder().build();
+
+ OMRequest omRequest = createOMRequest(Type.ServiceList)
+ .setServiceListRequest(req)
+ .build();
+
+ final ServiceListResponse resp = handleError(submitRequest(omRequest))
+ .getServiceListResponse();
+
+ return resp.getServiceInfoList().stream()
+ .map(ServiceInfo::getFromProtobuf)
+ .collect(Collectors.toList());
+
+ }
+
+ @Override
+ public ServiceInfoEx getServiceInfo() throws IOException {
+ ServiceListRequest req = ServiceListRequest.newBuilder().build();
+
+ OMRequest omRequest = createOMRequest(Type.ServiceList)
+ .setServiceListRequest(req)
+ .build();
+
+ final ServiceListResponse resp = handleError(submitRequest(omRequest))
+ .getServiceListResponse();
+
+ return new ServiceInfoEx(
+ resp.getServiceInfoList().stream()
+ .map(ServiceInfo::getFromProtobuf)
+ .collect(Collectors.toList()),
+ resp.getCaCertificate());
+ }
+
+ @Override
+ public boolean bootstrap(List<OMNodeInfo> omNodeInfos) throws IOException {
Review comment:
We are directly using proto classes, can we have helper classes like how
we have done for all other requests.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2707,6 +2728,57 @@ public ServiceInfoEx getServiceInfo() throws IOException
{
return new ServiceInfoEx(getServiceList(), caCertPem);
}
+ @Override
+ public boolean bootstrap(List<OMNodeInfo> omNodeInfos) throws IOException {
+ LOG.error("OM bootstrap command should be received via the RPC server");
+ throw new UnsupportedOperationException("OM bootstrap command should be " +
+ "received via the RPC server");
+ }
+
+ /**
+ * Add a new OM Node to the HA cluster.
+ */
+ public void addNewOMNode(String newOMNodeId) {
+ OMNodeDetails newOMNodeDetails = OMNodeDetails.getOMNodeDetailsFromConf(
+ getConfiguration(), getOMServiceId(), newOMNodeId);
+ if (newOMNodeDetails == null) {
+ // Load new configuration object to read in new peer information
+ setConfiguration(new OzoneConfiguration());
Review comment:
This is newly bootstrapped OM, so when it starts it has loaded new
config, why do we need to do this?
##########
File path:
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/admin/om/AddOMSubcommand.java
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.admin.om;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.net.NetUtils;
+import org.apache.hadoop.ozone.OmUtils;
+import org.apache.hadoop.ozone.client.OzoneClientException;
+import org.apache.hadoop.ozone.om.helpers.ServiceInfo;
+import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMNodeInfo;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRoleInfo;
+import org.apache.hadoop.util.StringUtils;
+import picocli.CommandLine;
+
+import java.util.concurrent.Callable;
+
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
+import static
org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_HTTPS_ADDRESS_KEY;
+import static
org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_HTTP_ADDRESS_KEY;
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_NODES_KEY;
+import static
org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_RATIS_PORT_DEFAULT;
+import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_RATIS_PORT_KEY;
+
+/**
+ * Handler of om roles command.
+ */
[email protected](
+ name = "addom", aliases = "addoms, addnewom, addnewoms",
+ description = "Adds newly Bootstrapped OM(s) to the cluster. " +
+ "\nNote - New OM(s) should be started in bootstrap mode before " +
+ "running this command." +
+ "\nNote - Configuration files of all existing " +
+ "OM(s) MUST be updated with information about the new OM(s) (" +
+ OZONE_OM_NODES_KEY + ", " + OZONE_OM_ADDRESS_KEY + ", " +
+ OZONE_OM_HTTP_ADDRESS_KEY + ", " + OZONE_OM_HTTPS_ADDRESS_KEY + ", " +
+ OZONE_OM_RATIS_PORT_KEY + ").",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class)
+public class AddOMSubcommand implements Callable<Void> {
+
+ @CommandLine.ParentCommand
+ private OMAdmin parent;
+
+ @CommandLine.Option(names = {"-id", "--service-id"},
+ description = "OM Service ID",
+ required = true)
+ private String omServiceId;
+
+ @CommandLine.Option(names = {"-nodeid", "-nodeids", "--new-node-ids"},
+ description = "A comma separated list of node IDs of the new OM(s). ",
+ required = true)
+ private String newNodeIds;
+
+ private OzoneManagerProtocol ozoneManagerClient;
+
+ @Override
+ public Void call() throws Exception {
+ boolean success = false;
+ try {
+ ozoneManagerClient = parent.createOmClient(omServiceId);
+
+ List<OMNodeInfo> newOMNodeInfos = getNewNodeInfos();
+
+ if (!newOMNodeInfos.isEmpty()) {
Review comment:
Here first we need to check for not null, as in few code paths
getNewNodeInfos returns null.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
