From 4a99186c901f026d04eeb69ed56d899913bfe92f Mon Sep 17 00:00:00 2001
From: Arpit Bandejiya <abandeji@amazon.com>
Date: Thu, 14 Mar 2024 15:41:39 +0530
Subject: [PATCH] Address review comments

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
---
 .../admin/cluster/node/stats/NodeStats.java   |  2 +-
 .../cluster/stats/ClusterStatsNodes.java      | 49 +++++++++---------
 .../recovery/PeerRecoverySourceService.java   |  1 +
 .../indices/recovery/PeerRecoveryStats.java   | 27 ++++++----
 .../recovery/PeerRecoveryStatsTracker.java    | 50 +++++++++----------
 .../StatsAwareReplicationCollection.java      | 10 ++--
 6 files changed, 73 insertions(+), 66 deletions(-)

diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodeStats.java b/server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodeStats.java
index 078010b7d93d0..27f7bee2c9e0e 100644
--- a/server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodeStats.java
+++ b/server/src/main/java/org/opensearch/action/admin/cluster/node/stats/NodeStats.java
@@ -239,7 +239,7 @@ public NodeStats(StreamInput in) throws IOException {
             admissionControlStats = null;
         }
 
-        if (in.getVersion().onOrAfter(Version.V_2_13_0)) {
+        if (in.getVersion().onOrAfter(Version.CURRENT)) {
             peerRecoveryStats = in.readOptionalWriteable(PeerRecoveryStats::new);
         } else {
             peerRecoveryStats = null;
diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodes.java b/server/src/main/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodes.java
index a8b7181f20566..8565074534b12 100644
--- a/server/src/main/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodes.java
+++ b/server/src/main/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodes.java
@@ -49,7 +49,6 @@
 import org.opensearch.core.xcontent.ToXContentFragment;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.discovery.DiscoveryModule;
-import org.opensearch.indices.recovery.PeerRecoveryStats;
 import org.opensearch.monitor.fs.FsInfo;
 import org.opensearch.monitor.jvm.JvmInfo;
 import org.opensearch.monitor.os.OsInfo;
@@ -861,40 +860,42 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
 
     static class PeerRecoveryStats implements ToXContentFragment {
 
-        private long total_started_recoveries;
-        private long total_failed_recoveries;
-        private long total_completed_recoveries;
-        private long total_retried_recoveries;
-        private long total_cancelled_recoveries;
-
-        private static final String TotalStartedRecoveries = "total_started_recoveries";
-        private static final String TotalFailedRecoveries = "total_failed_recoveries";
-        private static final String TotalCompletedRecoveries = "total_completed_recoveries";
-        private static final String TotalRetriedRecoveries = "total_retried_recoveries";
-        private static final String TotalCancelledRecoveries = "total_cancelled_recoveries";
+        private long totalStartedRecoveries;
+        private long totalFailedRecoveries;
+        private long totalCompletedRecoveries;
+        private long totalRetriedRecoveries;
+        private long totalCancelledRecoveries;
 
         PeerRecoveryStats(List<NodeStats> nodeStats) {
             for (NodeStats nodeStat : nodeStats) {
                 if (nodeStat.getPeerRecoveryStats() != null) {
-                    total_started_recoveries += nodeStat.getPeerRecoveryStats().getTotalStartedRecoveries();
-                    total_failed_recoveries += nodeStat.getPeerRecoveryStats().getTotalFailedRecoveries();
-                    total_completed_recoveries += nodeStat.getPeerRecoveryStats().getTotalCompletedRecoveries();
-                    total_retried_recoveries += nodeStat.getPeerRecoveryStats().getTotalRetriedRecoveries();
-                    total_cancelled_recoveries += nodeStat.getPeerRecoveryStats().getTotalCancelledRecoveries();
+                    totalStartedRecoveries += nodeStat.getPeerRecoveryStats().getTotalStartedRecoveries();
+                    totalFailedRecoveries += nodeStat.getPeerRecoveryStats().getTotalFailedRecoveries();
+                    totalCompletedRecoveries += nodeStat.getPeerRecoveryStats().getTotalCompletedRecoveries();
+                    totalRetriedRecoveries += nodeStat.getPeerRecoveryStats().getTotalRetriedRecoveries();
+                    totalCancelledRecoveries += nodeStat.getPeerRecoveryStats().getTotalCancelledRecoveries();
                 }
             }
         }
 
+        public static final class Fields {
+            static final String PEER_RECOVERY_STATS = "peer_recovery_stats";
+            static final String TOTAL_STARTED_RECOVERIES = "total_started_recoveries";
+            static final String TOTAL_FAILED_RECOVERIES = "total_failed_recoveries";
+            static final String TOTAL_COMPLETED_RECOVERIES = "total_completed_recoveries";
+            static final String TOTAL_RETRIED_RECOVERIES = "total_retried_recoveries";
+            static final String TOTAL_CANCELLED_RECOVERIES = "total_cancelled_recoveries";
+        }
+
         @Override
         public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException {
-            builder.startObject("peer_recovery_stats");
+            builder.startObject(Fields.PEER_RECOVERY_STATS);
             {
-                builder.field(TotalStartedRecoveries, total_started_recoveries);
-                builder.field(TotalFailedRecoveries, total_failed_recoveries);
-                builder.field(TotalCompletedRecoveries, total_completed_recoveries);
-                builder.field(TotalRetriedRecoveries, total_retried_recoveries);
-                builder.field(TotalCancelledRecoveries, total_cancelled_recoveries);
-
+                builder.field(Fields.TOTAL_STARTED_RECOVERIES, totalStartedRecoveries);
+                builder.field(Fields.TOTAL_FAILED_RECOVERIES, totalFailedRecoveries);
+                builder.field(Fields.TOTAL_COMPLETED_RECOVERIES, totalCompletedRecoveries);
+                builder.field(Fields.TOTAL_RETRIED_RECOVERIES, totalRetriedRecoveries);
+                builder.field(Fields.TOTAL_CANCELLED_RECOVERIES, totalCancelledRecoveries);
             }
             builder.endObject();
             return builder;
diff --git a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoverySourceService.java b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoverySourceService.java
index 43af8485e2631..cb2bedf00de99 100644
--- a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoverySourceService.java
+++ b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoverySourceService.java
@@ -165,6 +165,7 @@ private void recover(StartRecoveryRequest request, ActionListener<RecoveryRespon
         if (routingEntry.primary() == false || routingEntry.active() == false) {
             throw new DelayRecoveryException("source shard [" + routingEntry + "] is not an active primary");
         }
+
         if (request.isPrimaryRelocation()
             && (routingEntry.relocating() == false || routingEntry.relocatingNodeId().equals(request.targetNode().getId()) == false)) {
             logger.debug(
diff --git a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStats.java b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStats.java
index eb41660116ab6..4c24a3183b3fe 100644
--- a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStats.java
+++ b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStats.java
@@ -80,19 +80,24 @@ public long getTotalCancelledRecoveries() {
         return total_cancelled_recoveries;
     }
 
-    private static final String TotalStartedRecoveries = "total_started_recoveries";
-    private static final String TotalFailedRecoveries = "total_failed_recoveries";
-    private static final String TotalCompletedRecoveries = "total_completed_recoveries";
-    private static final String TotalRetriedRecoveries = "total_retried_recoveries";
-    private static final String TotalCancelledRecoveries = "total_cancelled_recoveries";
+    public static final class Fields {
+        static final String PEER_RECOVERY_STATS = "peer_recovery_stats";
+        static final String TOTAL_STARTED_RECOVERIES = "total_started_recoveries";
+        static final String TOTAL_FAILED_RECOVERIES = "total_failed_recoveries";
+        static final String TOTAL_COMPLETED_RECOVERIES = "total_completed_recoveries";
+        static final String TOTAL_RETRIED_RECOVERIES = "total_retried_recoveries";
+        static final String TOTAL_CANCELLED_RECOVERIES = "total_cancelled_recoveries";
+    }
 
     public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
-        builder.startObject("peer_recovery_stats");
-        builder.field(TotalStartedRecoveries, total_started_recoveries);
-        builder.field(TotalFailedRecoveries, total_failed_recoveries);
-        builder.field(TotalCompletedRecoveries, total_completed_recoveries);
-        builder.field(TotalRetriedRecoveries, total_retried_recoveries);
-        builder.field(TotalCancelledRecoveries, total_cancelled_recoveries);
+        builder.startObject(Fields.PEER_RECOVERY_STATS);
+        {
+            builder.field(Fields.TOTAL_STARTED_RECOVERIES, total_started_recoveries);
+            builder.field(Fields.TOTAL_FAILED_RECOVERIES, total_failed_recoveries);
+            builder.field(Fields.TOTAL_COMPLETED_RECOVERIES, total_completed_recoveries);
+            builder.field(Fields.TOTAL_RETRIED_RECOVERIES, total_retried_recoveries);
+            builder.field(Fields.TOTAL_CANCELLED_RECOVERIES, total_cancelled_recoveries);
+        }
         return builder.endObject();
     }
 }
diff --git a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStatsTracker.java b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStatsTracker.java
index 77f23e902cfd2..1713ac36990a5 100644
--- a/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStatsTracker.java
+++ b/server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryStatsTracker.java
@@ -16,57 +16,57 @@
  * @opensearch.internal
  */
 public class PeerRecoveryStatsTracker {
-    private final AtomicLong total_started_recoveries;
-    private final AtomicLong total_failed_recoveries;
-    private final AtomicLong total_completed_recoveries;
-    private final AtomicLong total_retried_recoveries;
-    private final AtomicLong total_cancelled_recoveries;
+    private final AtomicLong totalStartedRecoveries;
+    private final AtomicLong totalFailedRecoveries;
+    private final AtomicLong totalCompletedRecoveries;
+    private final AtomicLong totalRetriedRecoveries;
+    private final AtomicLong totalCancelledRecoveries;
 
     public PeerRecoveryStatsTracker() {
-        total_started_recoveries = new AtomicLong();
-        total_failed_recoveries = new AtomicLong();
-        total_completed_recoveries = new AtomicLong();
-        total_retried_recoveries = new AtomicLong();
-        total_cancelled_recoveries = new AtomicLong();
+        totalStartedRecoveries = new AtomicLong();
+        totalFailedRecoveries = new AtomicLong();
+        totalCompletedRecoveries = new AtomicLong();
+        totalRetriedRecoveries = new AtomicLong();
+        totalCancelledRecoveries = new AtomicLong();
     }
 
-    public void incrementTotalStartedRelocation(long increment) {
-        total_started_recoveries.addAndGet(increment);
+    public void incrementTotalStartedRecoveries(long increment) {
+        totalStartedRecoveries.addAndGet(increment);
     }
 
-    public void incrementTotalFailedRelocation(long increment) {
-        total_failed_recoveries.addAndGet(increment);
+    public void incrementTotalFailedRecoveries(long increment) {
+        totalFailedRecoveries.addAndGet(increment);
     }
 
-    public void incrementTotalCompletedRelocation(long increment) {
-        total_completed_recoveries.addAndGet(increment);
+    public void incrementTotalCompletedRecoveries(long increment) {
+        totalCompletedRecoveries.addAndGet(increment);
     }
 
-    public void incrementTotalRetriedRelocation(long increment) {
-        total_retried_recoveries.addAndGet(increment);
+    public void incrementTotalRetriedRecoveries(long increment) {
+        totalRetriedRecoveries.addAndGet(increment);
     }
 
-    public void incrementTotalCancelledRelocation(long increment) {
-        total_cancelled_recoveries.addAndGet(increment);
+    public void incrementTotalCancelledRecoveries(long increment) {
+        totalCancelledRecoveries.addAndGet(increment);
     }
 
     public long getTotalCancelledRecoveries() {
-        return total_cancelled_recoveries.get();
+        return totalCancelledRecoveries.get();
     }
 
     public long getTotalStartedRecoveries() {
-        return total_started_recoveries.get();
+        return totalStartedRecoveries.get();
     }
 
     public long getTotalFailedRecoveries() {
-        return total_failed_recoveries.get();
+        return totalFailedRecoveries.get();
     }
 
     public long getTotalCompletedRecoveries() {
-        return total_completed_recoveries.get();
+        return totalCompletedRecoveries.get();
     }
 
     public long getTotalRetriedRecoveries() {
-        return total_retried_recoveries.get();
+        return totalRetriedRecoveries.get();
     }
 }
diff --git a/server/src/main/java/org/opensearch/indices/replication/common/StatsAwareReplicationCollection.java b/server/src/main/java/org/opensearch/indices/replication/common/StatsAwareReplicationCollection.java
index a1bd7bdf0c5a7..796473114afe3 100644
--- a/server/src/main/java/org/opensearch/indices/replication/common/StatsAwareReplicationCollection.java
+++ b/server/src/main/java/org/opensearch/indices/replication/common/StatsAwareReplicationCollection.java
@@ -32,31 +32,31 @@ public StatsAwareReplicationCollection(Logger logger, ThreadPool threadPool) {
 
     @Override
     public boolean cancelForShard(ShardId shardId, String reason) {
-        tracker.incrementTotalCancelledRelocation(1);
+        tracker.incrementTotalCancelledRecoveries(1);
         return super.cancelForShard(shardId, reason);
     }
 
     @Override
     public long start(T target, TimeValue activityTimeout) {
-        tracker.incrementTotalStartedRelocation(1);
+        tracker.incrementTotalStartedRecoveries(1);
         return super.start(target, activityTimeout);
     }
 
     @Override
     public T reset(final long id, final TimeValue activityTimeout) {
-        tracker.incrementTotalRetriedRelocation(1);
+        tracker.incrementTotalRetriedRecoveries(1);
         return super.reset(id, activityTimeout);
     }
 
     @Override
     public void fail(long id, ReplicationFailedException e, boolean sendShardFailure) {
-        tracker.incrementTotalFailedRelocation(1);
+        tracker.incrementTotalFailedRecoveries(1);
         super.fail(id, e, sendShardFailure);
     }
 
     @Override
     public void markAsDone(long id) {
-        tracker.incrementTotalCompletedRelocation(1);
+        tracker.incrementTotalCompletedRecoveries(1);
         super.markAsDone(id);
     }