neils-dev commented on code in PR #4358:
URL: https://github.com/apache/ozone/pull/4358#discussion_r1140917359
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -355,6 +356,26 @@ public boolean addSCM(AddSCMRequest request) throws
IOException {
return getRatisServer().addSCM(request);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public boolean removeSCM(RemoveSCMRequest request) throws IOException {
+ // Currently we don't support decommission of Primordial SCM
+ // The caller should make sure that the requested node is not Primordial
SCM
+
+ String clusterId = scm.getClusterId();
+ if (!request.getClusterId().equals(scm.getClusterId())) {
+ throw new IOException("SCM " + request.getScmId() +
Review Comment:
Thanks @nandakumar131 for providing support for removing an SCM from the
ratis ring with this patch.
For clarity, would be helpful to add cause message for exception at start of
`IOException `message, `"Cannot remove primordial node from ring. SCM " +
request.getScmId() + ...`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java:
##########
@@ -320,6 +321,45 @@ public boolean addSCM(AddSCMRequest request) throws
IOException {
}
}
+ @Override
+ public boolean removeSCM(RemoveSCMRequest request) throws IOException {
+ final List<RaftPeer> newRaftPeerList =
+ new ArrayList<>(getDivision().getGroup().getPeers());
Review Comment:
instead of calling `getDivision()` here can we not just use the `division`
variable declared in the `SCMRatisServicerImpl`?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -2070,4 +2071,27 @@ private String reconfOzoneAdmins(String newVal) {
newVal, admins);
return String.valueOf(newVal);
}
+
+ /**
+ * This will remove the given SCM node from HA Ring by removing it from
+ * Ratis Ring and deleting the related certificates from certificate store.
+ *
+ * @return true if remove was successful, else false.
+ */
+ public boolean removePeerFromHARing(RemoveSCMRequest request)
+ throws IOException {
+ // We cannot remove a node if it's currently leader.
+ if (scmContext.isLeader() && request.getScmId().equals(getScmId())) {
+ throw new IOException("Cannot remove current leader.");
+ }
+
+ // Currently we don't support removal of primordial node.
+ if (request.getScmId().equals(primaryScmNodeId)) {
+ throw new IOException("Removal of primordial node is not supported.");
+ }
+
+ // TODO: Remove the certificate from certificate store.
Review Comment:
Thanks @nandakumar131 for the comment on certificate removal for node
removed from ratis ring. What needs to be done to remove certificate? Are we
blocked/waiting on something needed for this?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -2070,4 +2071,27 @@ private String reconfOzoneAdmins(String newVal) {
newVal, admins);
return String.valueOf(newVal);
}
+
+ /**
+ * This will remove the given SCM node from HA Ring by removing it from
+ * Ratis Ring and deleting the related certificates from certificate store.
+ *
+ * @return true if remove was successful, else false.
+ */
+ public boolean removePeerFromHARing(RemoveSCMRequest request)
+ throws IOException {
+ // We cannot remove a node if it's currently leader.
+ if (scmContext.isLeader() && request.getScmId().equals(getScmId())) {
Review Comment:
Q. If we need to perform maintenance on the _current leader_ SCM node, how
do we decommission it?
> // We cannot remove a node if it's currently leader.
--
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]
