Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public final class ScmInfo {
private final String clusterId;
private final String scmId;
private final List<String> peerRoles;
private final boolean scmRatisEnabled;

/**
* Builder for ScmInfo.
Expand All @@ -38,6 +39,7 @@ public static class Builder {
private String clusterId;
private String scmId;
private final List<String> peerRoles;
private boolean scmRatisEnabled;

public Builder() {
peerRoles = new ArrayList<>();
Expand Down Expand Up @@ -73,15 +75,28 @@ public Builder setRatisPeerRoles(List<String> roles) {
return this;
}

/**
* Set whether SCM enables Ratis.
*
* @param ratisEnabled If it is true, it means that the Ratis mode is turned on.
* If it is false, it means that the Ratis mode is not turned on.
* @return Builder for scmInfo
*/
public Builder setScmRatisEnabled(boolean ratisEnabled) {
scmRatisEnabled = ratisEnabled;
return this;
}

public ScmInfo build() {
return new ScmInfo(clusterId, scmId, peerRoles);
return new ScmInfo(clusterId, scmId, peerRoles, scmRatisEnabled);
}
}

private ScmInfo(String clusterId, String scmId, List<String> peerRoles) {
private ScmInfo(String clusterId, String scmId, List<String> peerRoles, boolean ratisEnabled) {
this.clusterId = clusterId;
this.scmId = scmId;
this.peerRoles = Collections.unmodifiableList(peerRoles);
this.scmRatisEnabled = ratisEnabled;
}

/**
Expand All @@ -107,4 +122,8 @@ public String getScmId() {
public List<String> getRatisPeerRoles() {
return peerRoles;
}

public boolean getScmRatisEnabled() {
return scmRatisEnabled;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,15 @@ StartContainerBalancerResponseProto startContainerBalancer(
*/
List<String> getScmRatisRoles() throws IOException;

/**
* Get the current SCM mode.
*
* @return `true` indicates that it is in RATIS mode,
* while `false` indicates that it is in STANDALONE mode.
* @throws IOException an I/O exception of some sort has occurred.
*/
boolean isScmRatisEnable() throws IOException;

/**
* Force generates new secret keys (rotate).
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@

import java.io.Closeable;
import java.io.IOException;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -157,6 +158,12 @@ public final class StorageContainerLocationProtocolClientSideTranslatorPB
private final StorageContainerLocationProtocolPB rpcProxy;
private final SCMContainerLocationFailoverProxyProvider fpp;

/**
* This is used to check if 'leader' or 'follower' exists,
* in order to confirm whether we have enabled Ratis.
*/
private final List<String> scmRatisRolesToCheck = Arrays.asList("leader", "follower");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable scmRatisRolesToCheck is only referenced in one place, and I'm not sure if defining it as a global variable is a good idea. Additionally, from my understanding, using static might be better if it needs to be used. @ChenSammi Please help give some suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@errose28 @adoroszlai May I take up some of your time to ask for some advice? Thank you very much!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whbing How should I modify this? Should I change the variable to uppercase and make it static?


/**
* Creates a new StorageContainerLocationProtocolClientSideTranslatorPB.
*
Expand Down Expand Up @@ -760,8 +767,23 @@ public ScmInfo getScmInfo() throws IOException {
.setScmId(resp.getScmId())
.setRatisPeerRoles(resp.getPeerRolesList());

return builder.build();
// By default, we assume that SCM Ratis is not enabled.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have considered whether the ScmRatisEnabled field exists in the proto. If the field is not present, we determine whether Ratis is enabled based on whether peerRolesList includes leader or follower.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChenSammi Kindly ping, Can you review this PR again? Thank you very much!


// If the response contains the `ScmRatisEnabled` field,
// we will set it directly; otherwise,
// we will determine if Ratis is enabled based on
// whether the `peerRolesList` contains the keywords 'leader' or 'follower'.
if (resp.hasScmRatisEnabled()) {
builder.setScmRatisEnabled(resp.getScmRatisEnabled());
} else {
List<String> peerRolesList = resp.getPeerRolesList();
if (!peerRolesList.isEmpty()) {
boolean containsScmRoles = peerRolesList.stream().map(String::toLowerCase)
.anyMatch(scmRatisRolesToCheck::contains);
builder.setScmRatisEnabled(containsScmRoles);
}
}
return builder.build();
}

@Override
Expand Down
1 change: 1 addition & 0 deletions hadoop-hdds/interface-client/src/main/proto/hdds.proto
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ message GetScmInfoResponseProto {
required string clusterId = 1;
required string scmId = 2;
repeated string peerRoles = 3;
optional bool scmRatisEnabled = 4;
}

message AddScmRequestProto {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -837,13 +837,15 @@ public ScmInfo getScmInfo() {
if (scm.getScmHAManager().getRatisServer() != null) {
builder.setRatisPeerRoles(
scm.getScmHAManager().getRatisServer().getRatisRoles());
builder.setScmRatisEnabled(true);
} else {
// In case, there is no ratis, there is no ratis role.
// This will just print the hostname with ratis port as the default
// behaviour.
String address = scm.getSCMHANodeDetails().getLocalNodeDetails()
.getRatisHostPortStr();
builder.setRatisPeerRoles(Arrays.asList(address));
builder.setScmRatisEnabled(false);
}
return builder.build();
} catch (Exception ex) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,11 @@ public List<String> getScmRatisRoles() throws IOException {
return storageContainerLocationClient.getScmInfo().getRatisPeerRoles();
}

@Override
public boolean isScmRatisEnable() throws IOException {
return storageContainerLocationClient.getScmInfo().getScmRatisEnabled();
}

@Override
public boolean rotateSecretKeys(boolean force) throws IOException {
return secretKeyClient.checkAndRotate(force);
Expand Down
6 changes: 5 additions & 1 deletion hadoop-ozone/dist/src/main/smoketest/admincli/scmrole.robot
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ Run scm roles
List scm roles as JSON
${output} = Execute ozone admin scm roles --json
${leader} = Execute echo '${output}' | jq -r '.[] | select(.raftPeerRole == "LEADER")'
Should Not Be Equal ${leader} ${EMPTY}
Should Not Be Equal ${leader} ${EMPTY}

List scm roles as TABLE
${output} = Execute ozone admin scm roles --table
Should Match Regexp ${output} \\|.*LEADER.*
15 changes: 15 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/omha/om-roles.robot
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ Assert Leader Present in JSON
[Arguments] ${output}
${leader} = Execute echo '${output}' | jq '.[] | select(.[] | .serverRole == "LEADER")'
Should Not Be Equal ${leader} ${EMPTY}
Assert Leader Present in TABLE
[Arguments] ${output}
Should Match Regexp ${output} \\|.*LEADER.*

*** Test Cases ***
List om roles with OM service ID passed
Expand All @@ -53,3 +56,15 @@ List om roles as JSON without OM service ID passed
Assert Leader Present in JSON ${output_without_id_passed}
${output_without_id_passed} = Execute And Ignore Error ozone admin --set=ozone.om.service.ids=omservice,omservice2 om roles --json
Should Contain ${output_without_id_passed} no Ozone Manager service ID specified

List om roles as TABLE with OM service ID passed
${output_with_id_passed} = Execute ozone admin om roles --service-id=omservice --table
Assert Leader Present in TABLE ${output_with_id_passed}
${output_with_id_passed} = Execute ozone admin --set=ozone.om.service.ids=omservice,omservice2 om roles --service-id=omservice --table
Assert Leader Present in TABLE ${output_with_id_passed}

List om roles as TABLE without OM service ID passed
${output_without_id_passed} = Execute ozone admin om roles --table
Assert Leader Present in TABLE ${output_without_id_passed}
${output_without_id_passed} = Execute And Ignore Error ozone admin --set=ozone.om.service.ids=omservice,omservice2 om roles --table
Should Contain ${output_without_id_passed} no Ozone Manager service ID specified
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,20 @@

package org.apache.hadoop.ozone.admin.om;

import org.apache.hadoop.classification.VisibleForTesting;
import org.apache.hadoop.hdds.cli.HddsVersionProvider;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.server.JsonUtils;
import org.apache.hadoop.ozone.client.OzoneClientException;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRoleInfo;
import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
import org.apache.hadoop.ozone.om.helpers.ServiceInfo;
import org.apache.hadoop.ozone.utils.FormattingCLIUtils;
import picocli.CommandLine;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -57,14 +60,37 @@ public class GetServiceRolesSubcommand implements Callable<Void> {
description = "Format output as JSON")
private boolean json;

@CommandLine.Option(names = { "--table" },
defaultValue = "false",
description = "Format output as Table")
private boolean table;

private OzoneManagerProtocol ozoneManagerClient;

private static final String OM_ROLES_TITLE = "Ozone Manager Roles";

private static final List<String> OM_ROLES_HEADER = Arrays.asList(
"Host Name", "Node ID", "Role");

@Override
public Void call() throws Exception {
try {
ozoneManagerClient = parent.createOmClient(omServiceId);
if (json) {
printOmServerRolesAsJson(ozoneManagerClient.getServiceList());
} else if (table) {
FormattingCLIUtils formattingCLIUtils = new FormattingCLIUtils(OM_ROLES_TITLE)
.addHeaders(OM_ROLES_HEADER);
List<ServiceInfo> serviceList = ozoneManagerClient.getServiceList();
for (ServiceInfo serviceInfo : serviceList) {
OMRoleInfo omRoleInfo = serviceInfo.getOmRoleInfo();
if (omRoleInfo != null &&
serviceInfo.getNodeType() == HddsProtos.NodeType.OM) {
formattingCLIUtils.addLine(new String[]{serviceInfo.getHostname(),
omRoleInfo.getNodeId(), omRoleInfo.getServerRole()});
}
}
System.out.println(formattingCLIUtils.render());
} else {
printOmServerRoles(ozoneManagerClient.getServiceList());
}
Expand Down Expand Up @@ -110,4 +136,14 @@ private void printOmServerRolesAsJson(List<ServiceInfo> serviceList)
System.out.print(
JsonUtils.toJsonStringWithDefaultPrettyPrinter(omServiceList));
}

@VisibleForTesting
public void setOzoneManagerClient(OzoneManagerProtocol ozoneManagerClient) {
this.ozoneManagerClient = ozoneManagerClient;
}

@VisibleForTesting
public void setParent(OMAdmin parent) {
this.parent = parent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.ozone.admin.scm;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand All @@ -27,6 +28,7 @@
import org.apache.hadoop.hdds.scm.cli.ScmSubcommand;
import org.apache.hadoop.hdds.scm.client.ScmClient;
import org.apache.hadoop.hdds.server.JsonUtils;
import org.apache.hadoop.ozone.utils.FormattingCLIUtils;
import picocli.CommandLine;

import static java.lang.System.err;
Expand All @@ -50,13 +52,44 @@ public class GetScmRatisRolesSubcommand extends ScmSubcommand {
description = "Format output as JSON")
private boolean json;

@CommandLine.Option(names = { "--table" },
defaultValue = "false",
description = "Format output as Table")
private boolean table;

private static final String SCM_ROLES_TITLE = "Storage Container Manager Roles";

private static final List<String> RATIS_SCM_ROLES_HEADER = Arrays.asList(
"Host Name", "Ratis Port", "Role", "Node ID", "Host Address");

private static final List<String> STANDALONE_SCM_ROLES_HEADER = Arrays.asList("Host Name", "Port");

@Override
protected void execute(ScmClient scmClient) throws IOException {
public void execute(ScmClient scmClient) throws IOException {
List<String> ratisRoles = scmClient.getScmRatisRoles();
boolean isRatisEnabled = scmClient.isScmRatisEnable();
if (json) {
Map<String, Map<String, String>> scmRoles = parseScmRoles(ratisRoles);
System.out.print(
JsonUtils.toJsonStringWithDefaultPrettyPrinter(scmRoles));
} else if (table) {
FormattingCLIUtils formattingCLIUtils = new FormattingCLIUtils(SCM_ROLES_TITLE);

// Determine which header to use based on whether Ratis is enabled or not.
if (isRatisEnabled) {
formattingCLIUtils.addHeaders(RATIS_SCM_ROLES_HEADER);
Copy link
Contributor Author

@slfan1989 slfan1989 Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whbing @ChenSammi

@slfan1989 , thanks for improving the patch. For this scmHaEnabled, can you explain a little bit about its purpose?

Thank you very much for reviewing this PR! I’d like to explain why I added the scmRatisEnable:

When using --table to output SCM information, we need to first determine the length of the table headers.

For non-HA SCMs, we will display it in the following format:

+-----------------------------------------------+
|        Storage Container Manager Roles        | 
+---------------------------------+-------------+
|            Host Name            |     Port    | 
+---------------------------------+-------------+

For HA SCMs, we will display it in the following format:

+---------------------------------------------------------------------------------------------------------------+
|                                        Storage Container Manager Roles                                        |
+---------------------------------+------------+----------+--------------------------------------+--------------+
|            Host Name            | Ratis Port | Role     |                 Node ID              | Host Address |
+---------------------------------+------------+----------+--------------------------------------+--------------+

I need a clear method to determine if SCM has Ratis enabled.

I am considering two approaches:

Approach 1: Use the ratisRole length of the returned data for judgment.
Approach 2: Add a return attribute that explicitly indicates whether Ratis is being used.

From my perspective, Approach 2 is better, as the code looks clearer.

} else {
formattingCLIUtils.addHeaders(STANDALONE_SCM_ROLES_HEADER);
}

for (String role : ratisRoles) {
String[] roleItems = role.split(":");
if (roleItems.length < 2) {
err.println("Invalid response received for ScmRatisRoles.");
}
formattingCLIUtils.addLine(roleItems);
}
System.out.println(formattingCLIUtils.render());
} else {
for (String role: ratisRoles) {
System.out.println(role);
Expand Down
Loading