HDDS-16130. Remove Pair usage from Safe-mode APIs and callers. by navinko · Pull Request #10993 · apache/ozone

navinko · 2026-08-11T18:49:01Z

What changes were proposed in this pull request?

This PR splits out the safe-mode API and caller updates from the larger HDDS-15942 change set in PR #10962 into a separate subtask.

Why are the changes needed?

It replaces the old Pair<Boolean, String> based safe-mode status contract with the new SafeModeRuleStatus type across the SCM protocol, SCM server, and cli-admin. Replacing it with SafeModeRuleStatus gives the fields a clear meaning, self-describing and easier to maintain.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16130

How was this patch tested?

Updated the respective Test cases and validated locally.
Successful CI : https://github.com/navinko/ozone/actions/runs/31507651933

navinko · 2026-08-11T19:21:39Z

Hi @szetszwo , Kindly review once you get some time.

Gargi-jais11 · 2026-08-12T05:09:31Z

@sreejasahithi would u like to review this?

szetszwo

@navinko , thanks for working on this! Since there is already a SafeModeRuleStatusProto, let's just use it and do not add SafeModeRuleStatus.

BTW, Ozone use 120 characters for line width. Please combine the existing short lines when you are changing the related code.

szetszwo · 2026-08-14T04:09:27Z

+/**
+ * Status of SCM safe mode exit rule.
+ */
+public final class SafeModeRuleStatus {


In this case, do not add SafeModeRuleStatus. Just use SafeModeRuleStatusProto.

Thanks @szetszwo for the review
My bad , please allow me to correct and update the PR

navinko · 2026-08-15T07:01:42Z

Thanks @szetszwo for the review .
I tried addressing the suggestions. kindly review once you get some time.
Successful CI : https://github.com/navinko/ozone/actions/runs/31826571802

szetszwo

@navinko , thanks for the update!

Using Map in this case in unnecessary (since get(key) is never called) and inefficient. Let's change them to List; see https://issues.apache.org/jira/secure/attachment/13083934/10993_review.patch

navinko · 2026-08-16T21:22:40Z

@navinko , thanks for the update!

Using Map in this case in unnecessary (since get(key) is never called) and inefficient. Let's change them to List; see https://issues.apache.org/jira/secure/attachment/13083934/10993_review.patch

Thanks @szetszwo for the review.
Updated the changes as per review patch. Switched the safe-mode status flow to List end-to-end and removed the unnecessary map conversion.

Successful CI : https://github.com/navinko/ozone/actions/runs/31964576589/job/95219220076

szetszwo · 2026-08-16T22:38:37Z

-      String host1 = HddsUtils.getHostName(address1).orElse(address1);
-      String host2 = HddsUtils.getHostName(address2).orElse(address2);
-
-      boolean hostsMatch = host1.equalsIgnoreCase(host2);
-      if (!hostsMatch) {
-        InetAddress inet1 = InetAddress.getByName(host1);
-        InetAddress inet2 = InetAddress.getByName(host2);
-        hostsMatch = inet1.equals(inet2);
-      }
-      if (!hostsMatch) {
+      // Parse both addresses into host:port components
+      String[] parts1 = address1.split(":", 2);
+      String[] parts2 = address2.split(":", 2);
+
+      String host1 = parts1[0];
+      String host2 = parts2[0];
+
+      // Hostnames must match
+      if (!host1.equalsIgnoreCase(host2)) {


@navinko , This change is unrelated. If there is a need, let's do it separately.

The PR looks good other than that.

I got these unrelated changes post resolving conflict and accepted changes from master .

@navinko I think you might have accepted wrong changes
as this is the latest commit to this particular file SafeModeCheckSubcommand.java
https://github.com/apache/ozone/pull/10831/changes#diff-1ef0fa9c02403bdd29621610c5d5ee82c08a9a2b5dc96de3e738a9568b7feb42L188-R207

Could you please check this out.

Thanks @sreejasahithi for review , Ideally once we resolve the conflict we shud accept the changes coming from master . I also did the same , But instead of rebasing on master and resolving locally, used a shortcut and resolved the conflict in the GitHub UI , that merged master's changes to SafeModeCheckSubcommand.java into my diff and caused the unrelated changes. I've now rebased properly on the latest master and addressed the review comments. The PR now contains only the safe-mode changes and is List-based end-to-end, with no conflicts.

CI Build already triggered , will monitor and mark the PR for review.

sreejasahithi

Thanks @navinko for this improvement
overall changes LGTM

sreejasahithi · 2026-08-17T04:23:20Z

-      String host1 = HddsUtils.getHostName(address1).orElse(address1);
-      String host2 = HddsUtils.getHostName(address2).orElse(address2);
-
-      boolean hostsMatch = host1.equalsIgnoreCase(host2);
-      if (!hostsMatch) {
-        InetAddress inet1 = InetAddress.getByName(host1);
-        InetAddress inet2 = InetAddress.getByName(host2);
-        hostsMatch = inet1.equals(inet2);
-      }
-      if (!hostsMatch) {
+      // Parse both addresses into host:port components
+      String[] parts1 = address1.split(":", 2);
+      String[] parts2 = address2.split(":", 2);
+
+      String host1 = parts1[0];
+      String host2 = parts2[0];
+
+      // Hostnames must match
+      if (!host1.equalsIgnoreCase(host2)) {


@navinko I think you might have accepted wrong changes
as this is the latest commit to this particular file SafeModeCheckSubcommand.java
https://github.com/apache/ozone/pull/10831/changes#diff-1ef0fa9c02403bdd29621610c5d5ee82c08a9a2b5dc96de3e738a9568b7feb42L188-R207

Could you please check this out.

sreejasahithi · 2026-08-17T04:30:16Z

   * @return map of rule statuses.
   */


Could you please also update this comment as it is stale now.

sreejasahithi

Thanks for the update @navinko . The List change in the verbose path looks correct and matches the review feedback.

However, commit 1dd5fc4 changed too much in SafeModeCheckSubcommand.java. While trying to remove unrelated changes, it also removed findLeaderNode() and the HA branch in executeForSingleNode() and printSafeModeRules(). That logic is already on master so please restore it.

Also, matchesAddress() should stay as on master (HddsUtils.getHostName() + InetAddress for IPv6), not the older split(":") version.
For this file, the intended change is only in the verbose block inside queryNode() and also printSafeModeRules():

before: Map<String, Pair<Boolean, String>>
after: List<SafeModeRuleStatusProto>
Everything else in SafeModeCheckSubcommand.java should match master.

sreejasahithi · 2026-08-17T17:24:04Z


  private void executeForSingleNode(ScmClient scmClient, ScmNodeTarget targetScmNode) throws IOException {
-    SCMNodeInfo targetNode;
-    if (serviceId != null) {
-      // HA mode: find leader
-      targetNode = findLeaderNode(scmClient);
-      if (targetNode == null) {
-        throw new IOException("Could not determine leader node");
-      }
-    } else {
-      // Non-HA mode: use single node
-      targetNode = nodes.get(0);


Why was this code removed?

Thanks the PR was in DRAFT and changes in flight .

sreejasahithi · 2026-08-17T17:24:37Z

  }

-  /**
-   * Find the leader node from SCM roles.
-   * @param scmClient the SCM client
-   * @return the leader SCMNodeInfo
-   */
-  private SCMNodeInfo findLeaderNode(ScmClient scmClient) throws IOException {
-    try {
-      List<String> roles = scmClient.getScmRoles();
-      for (String role : roles) {
-        String[] parts;
-        try {
-          parts = HddsUtils.parseRatisRoleString(role);
-        } catch (IllegalArgumentException e) {
-          continue;
-        }
-        if (!"LEADER".equalsIgnoreCase(parts[2])) {
-          continue;
-        }
-        String leaderHost = parts[0];
-        String leaderIp = parts[4];
-        for (SCMNodeInfo node : nodes) {
-          String nodeHost = HddsUtils.getHostName(node.getScmClientAddress()).orElse("");
-
-          if (matchesAddress(leaderHost, nodeHost) || (!leaderIp.isEmpty() &&
-                  matchesAddress(leaderIp, nodeHost))) {
-            return node;
-          }
-        }
-      }


why was this code removed?

Thanks the PR was in DRAFT and changes in flight .

navinko · 2026-08-18T15:34:43Z

Thanks @sreejasahithi , @szetszwo for the review. I fixed the unintentional changes and PR is ready to review.
Successfull CI : https://github.com/navinko/ozone/actions/runs/32058141015

szetszwo

+1 the change looks good.

sreejasahithi

Thanks @navinko for updating the PR
LGTM

nit: (non-blocking) javadoc in ScmClient still says returns map for getSafeModeRuleStatuses

navinko · 2026-08-18T17:31:46Z

Thanks @navinko for updating the PR LGTM

nit: (non-blocking) javadoc in ScmClient still says returns map for getSafeModeRuleStatuses

Thanks @sreejasahithi my bad , corrected doc.

szetszwo · 2026-08-19T16:26:52Z

navinko force-pushed the HDDS-16130 branch from b5ca8de to 571d322 12 hours ago

@navinko , after force-pushed, reviewers cannot easily tell what are the new changes from the last review. So, please don't force-pushed since you are also forcing reviewers reading the changes from the beginning.

navinko · 2026-08-19T16:30:09Z

Sure @szetszwo will make sure going forward. I saw new conflict today so rebased and force push.

Gargi-jais11 · 2026-08-19T18:40:55Z

Sure @szetszwo will make sure going forward. I saw new conflict today so rebased and force push.

@navinko you can use git --merge master instead of git --rebase to avoid the force push in case of conflicts.
git pull --merge apache master

navinko · 2026-08-19T18:45:55Z

Sure @szetszwo will make sure going forward. I saw new conflict today so rebased and force push.

@navinko you can use git --merge master instead of git --rebase to avoid the force push in case of conflicts.
git pull --merge apache master

Thanks @Gargi-jais11 sure!

szetszwo · 2026-08-19T21:33:27Z

... merge master instead of git --rebase to avoid the force push in case of conflicts.

Please do merge instead of force push. If there is a need, please ask on the PR first before force-pushing it. As a reviewer, I really want to minimize my time on reviwing.

navinko · 2026-08-20T18:40:24Z

... merge master instead of git --rebase to avoid the force push in case of conflicts.

Please do merge instead of force push. If there is a need, please ask on the PR first before force-pushing it. As a reviewer, I really want to minimize my time on reviwing.

Thanks @szetszwo, makes sense. My intent was just to keep the history clean, but now am aware that it forces reviewers to re-read everything from the beginning. I'll follow the suggested approach going forward .
Also in this PR particularly, do we need to redo rebase ?

szetszwo

+1 the change looks good.

szetszwo · 2026-08-20T18:45:36Z

@sreejasahithi , @Gargi-jais11 , thanks also for reviewing this!

navinko · 2026-08-20T18:48:03Z

Thanks @sreejasahithi @Gargi-jais11 @szetszwo for reviewing and merging the PR.

Gargi-jais11 requested review from sadanand48 and szetszwo August 12, 2026 04:45

szetszwo reviewed Aug 14, 2026

View reviewed changes

navinko marked this pull request as draft August 14, 2026 04:24

navinko force-pushed the HDDS-16130 branch from 2c84403 to b2d313a Compare August 14, 2026 17:59

navinko marked this pull request as ready for review August 15, 2026 07:00

navinko requested a review from szetszwo August 15, 2026 18:20

szetszwo reviewed Aug 15, 2026

View reviewed changes

szetszwo mentioned this pull request Aug 15, 2026

HDDS-14108. Provide option in ‘scm safemode status’ to show status of all SCM nodes #9611

Merged

navinko marked this pull request as draft August 16, 2026 16:15

navinko marked this pull request as ready for review August 16, 2026 21:23

navinko requested a review from szetszwo August 16, 2026 21:23

szetszwo reviewed Aug 16, 2026

View reviewed changes

sreejasahithi reviewed Aug 17, 2026

View reviewed changes

navinko force-pushed the HDDS-16130 branch 3 times, most recently from 088cbb2 to 1dd5fc4 Compare August 17, 2026 17:00

navinko marked this pull request as draft August 17, 2026 17:38

sreejasahithi reviewed Aug 17, 2026

View reviewed changes

navinko force-pushed the HDDS-16130 branch 2 times, most recently from 2fa7167 to 434462e Compare August 17, 2026 18:03

navinko marked this pull request as ready for review August 17, 2026 19:10

navinko marked this pull request as draft August 17, 2026 19:18

navinko marked this pull request as ready for review August 18, 2026 15:34

szetszwo approved these changes Aug 18, 2026

View reviewed changes

navinko requested review from sreejasahithi and szetszwo August 18, 2026 16:29

sreejasahithi approved these changes Aug 18, 2026

View reviewed changes

navinko added 3 commits August 19, 2026 09:17

HDDS-16130. Addressed review comments

d6ab1e0

HDDS-16130. Addressed review comments and removed unused imports.

e6846a9

HDDS-16130. Corrected doc

571d322

navinko force-pushed the HDDS-16130 branch from b5ca8de to 571d322 Compare August 19, 2026 03:54

szetszwo approved these changes Aug 20, 2026

View reviewed changes

szetszwo merged commit 9e7ba24 into apache:master Aug 20, 2026
45 checks passed

Uh oh!

Conversation

navinko commented Aug 11, 2026

What changes were proposed in this pull request?

Why are the changes needed?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

navinko commented Aug 11, 2026

Uh oh!

Gargi-jais11 commented Aug 12, 2026

Uh oh!

szetszwo left a comment

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!

navinko commented Aug 15, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

navinko commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sreejasahithi left a comment

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!

sreejasahithi left a comment

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!

navinko commented Aug 18, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

sreejasahithi left a comment

Choose a reason for hiding this comment

Uh oh!

navinko commented Aug 18, 2026

Uh oh!

szetszwo commented Aug 19, 2026

Uh oh!

navinko commented Aug 19, 2026

Uh oh!

Gargi-jais11 commented Aug 19, 2026

Uh oh!

navinko commented Aug 19, 2026

Uh oh!

szetszwo commented Aug 19, 2026

Uh oh!

navinko commented Aug 20, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

navinko commented Aug 16, 2026 •

edited

Loading

szetszwo commented Aug 20, 2026 •

edited

Loading