Refactor TranslogStats and RequestCacheStats with Builder pattern (#19961)

pado0 · web-flow · commit e02b778eb04d · 2025-11-13T12:20:45.000-08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -52,6 +52,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Refactor the FieldDataStats and CompletionStats class to use the Builder pattern instead of constructors ([#19936](https://github.com/opensearch-project/OpenSearch/pull/19936))
 - Thread Context Preservation by gRPC Interceptor ([#19776](https://github.com/opensearch-project/OpenSearch/pull/19776))
 - Update NoOpResult constructors in the Engine to be public ([#19950](https://github.com/opensearch-project/OpenSearch/pull/19950))
+- Refactor the TranslogStats and RequestCacheStats class to use the Builder pattern instead of constructors ([#19961](https://github.com/opensearch-project/OpenSearch/pull/19961))
 
 ### Fixed
 - Fix Allocation and Rebalance Constraints of WeightFunction are incorrectly reset ([#19012](https://github.com/opensearch-project/OpenSearch/pull/19012))
@@ -108,6 +109,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Deprecated existing constructors in RemoteTranslogTransferTracker.Stats and RemoteSegmentTransferTracker.Stats in favor of the new Builder ([#19837](https://github.com/opensearch-project/OpenSearch/pull/19837))
 - Deprecated existing constructors in GetStats, FlushStats and QueryCacheStats in favor of the new Builder ([#19935](https://github.com/opensearch-project/OpenSearch/pull/19935))
 - Deprecated existing constructors in FieldDataStats and CompletionStats in favor of the new Builder ([#19936](https://github.com/opensearch-project/OpenSearch/pull/19936))
+- Deprecated existing constructors in TranslogStats and RequestCacheStats in favor of the new Builder ([#19961](https://github.com/opensearch-project/OpenSearch/pull/19961))
 
 ### Removed
 
diff --git a/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java b/server/src/main/java/org/opensearch/index/cache/request/RequestCacheStats.java
@@ -57,13 +57,30 @@ public class RequestCacheStats implements Writeable, ToXContentFragment {
 
     public RequestCacheStats() {}
 
+    /**
+     * Private constructor that takes a builder.
+     * This is the sole entry point for creating a new RequestCacheStats object.
+     * @param builder The builder instance containing all the values.
+     */
+    private RequestCacheStats(Builder builder) {
+        this.memorySize = builder.memorySize;
+        this.evictions = builder.evictions;
+        this.hitCount = builder.hitCount;
+        this.missCount = builder.missCount;
+    }
+
     public RequestCacheStats(StreamInput in) throws IOException {
         memorySize = in.readVLong();
         evictions = in.readVLong();
         hitCount = in.readVLong();
         missCount = in.readVLong();
     }
 
+    /**
+     * This constructor will be deprecated starting in version 3.4.0.
+     * Use {@link Builder} instead.
+     */
+    @Deprecated
     public RequestCacheStats(long memorySize, long evictions, long hitCount, long missCount) {
         this.memorySize = memorySize;
         this.evictions = evictions;
@@ -98,6 +115,47 @@ public long getMissCount() {
         return this.missCount;
     }
 
+    /**
+     * Builder for the {@link RequestCacheStats} class.
+     * Provides a fluent API for constructing a RequestCacheStats object.
+     */
+    public static class Builder {
+        private long memorySize = 0;
+        private long evictions = 0;
+        private long hitCount = 0;
+        private long missCount = 0;
+
+        public Builder() {}
+
+        public Builder memorySize(long count) {
+            this.memorySize = count;
+            return this;
+        }
+
+        public Builder evictions(long count) {
+            this.evictions = count;
+            return this;
+        }
+
+        public Builder hitCount(long count) {
+            this.hitCount = count;
+            return this;
+        }
+
+        public Builder missCount(long count) {
+            this.missCount = count;
+            return this;
+        }
+
+        /**
+         * Creates a {@link RequestCacheStats} object from the builder's current state.
+         * @return A new RequestCacheStats instance.
+         */
+        public RequestCacheStats build() {
+            return new RequestCacheStats(this);
+        }
+    }
+
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeVLong(memorySize);
diff --git a/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java b/server/src/main/java/org/opensearch/index/cache/request/ShardRequestCache.java
@@ -54,7 +54,11 @@ public final class ShardRequestCache {
     final CounterMetric missCount = new CounterMetric();
 
     public RequestCacheStats stats() {
-        return new RequestCacheStats(Math.max(0, totalMetric.count()), evictionsMetric.count(), hitCount.count(), missCount.count());
+        return new RequestCacheStats.Builder().memorySize(Math.max(0, totalMetric.count()))
+            .evictions(evictionsMetric.count())
+            .hitCount(hitCount.count())
+            .missCount(missCount.count())
+            .build();
     }
 
     public void onHit() {
diff --git a/server/src/main/java/org/opensearch/index/translog/LocalTranslog.java b/server/src/main/java/org/opensearch/index/translog/LocalTranslog.java
@@ -166,13 +166,12 @@ public TranslogStats stats() {
         // acquire lock to make the two numbers roughly consistent (no file change half way)
         try (ReleasableLock lock = readLock.acquire()) {
             long uncommittedGen = getMinGenerationForSeqNo(deletionPolicy.getLocalCheckpointOfSafeCommit() + 1).translogFileGeneration;
-            return new TranslogStats(
-                totalOperations(),
-                sizeInBytes(),
-                totalOperationsByMinGen(uncommittedGen),
-                sizeInBytesByMinGen(uncommittedGen),
-                earliestLastModifiedAge()
-            );
+            return new TranslogStats.Builder().numberOfOperations(totalOperations())
+                .translogSizeInBytes(sizeInBytes())
+                .uncommittedOperations(totalOperationsByMinGen(uncommittedGen))
+                .uncommittedSizeInBytes(sizeInBytesByMinGen(uncommittedGen))
+                .earliestLastModifiedAge(earliestLastModifiedAge())
+                .build();
         }
     }
 
diff --git a/server/src/main/java/org/opensearch/index/translog/Translog.java b/server/src/main/java/org/opensearch/index/translog/Translog.java
@@ -937,13 +937,12 @@ public TranslogStats stats() {
         // acquire lock to make the two numbers roughly consistent (no file change half way)
         try (ReleasableLock lock = readLock.acquire()) {
             long uncommittedGen = getMinGenerationForSeqNo(deletionPolicy.getLocalCheckpointOfSafeCommit() + 1).translogFileGeneration;
-            return new TranslogStats(
-                totalOperations(),
-                sizeInBytes(),
-                totalOperationsByMinGen(uncommittedGen),
-                sizeInBytesByMinGen(uncommittedGen),
-                earliestLastModifiedAge()
-            );
+            return new TranslogStats.Builder().numberOfOperations(totalOperations())
+                .translogSizeInBytes(sizeInBytes())
+                .uncommittedOperations(totalOperationsByMinGen(uncommittedGen))
+                .uncommittedSizeInBytes(sizeInBytesByMinGen(uncommittedGen))
+                .earliestLastModifiedAge(earliestLastModifiedAge())
+                .build();
         }
     }
 
diff --git a/server/src/main/java/org/opensearch/index/translog/TranslogStats.java b/server/src/main/java/org/opensearch/index/translog/TranslogStats.java
@@ -67,6 +67,20 @@ public TranslogStats() {
         remoteTranslogStats = new RemoteTranslogStats();
     }
 
+    /**
+     * Private constructor that takes a builder.
+     * This is the sole entry point for creating a new TranslogStats object.
+     * @param builder The builder instance containing all the values.
+     */
+    private TranslogStats(Builder builder) {
+        this.translogSizeInBytes = builder.translogSizeInBytes;
+        this.numberOfOperations = builder.numberOfOperations;
+        this.uncommittedSizeInBytes = builder.uncommittedSizeInBytes;
+        this.uncommittedOperations = builder.uncommittedOperations;
+        this.earliestLastModifiedAge = builder.earliestLastModifiedAge;
+        this.remoteTranslogStats = builder.remoteTranslogStats;
+    }
+
     public TranslogStats(StreamInput in) throws IOException {
         numberOfOperations = in.readVInt();
         translogSizeInBytes = in.readVLong();
@@ -78,6 +92,11 @@ public TranslogStats(StreamInput in) throws IOException {
             : new RemoteTranslogStats();
     }
 
+    /**
+     * This constructor will be deprecated starting in version 3.4.0.
+     * Use {@link TranslogStats.Builder} instead.
+     */
+    @Deprecated
     public TranslogStats(
         int numberOfOperations,
         long translogSizeInBytes,
@@ -159,6 +178,69 @@ public RemoteTranslogStats getRemoteTranslogStats() {
         return remoteTranslogStats;
     }
 
+    /**
+     * Builder for the {@link TranslogStats} class.
+     * Provides a fluent API for constructing a TranslogStats object.
+     */
+    public static class Builder {
+        private int numberOfOperations = 0;
+        private long translogSizeInBytes = 0;
+        private int uncommittedOperations = 0;
+        private long uncommittedSizeInBytes = 0;
+        private long earliestLastModifiedAge = 0;
+        private final RemoteTranslogStats remoteTranslogStats = new RemoteTranslogStats();
+
+        public Builder() {}
+
+        public Builder numberOfOperations(int operations) {
+            if (operations < 0) {
+                throw new IllegalArgumentException("numberOfOperations must be >= 0");
+            }
+            this.numberOfOperations = operations;
+            return this;
+        }
+
+        public Builder translogSizeInBytes(long size) {
+            if (size < 0) {
+                throw new IllegalArgumentException("translogSizeInBytes must be >= 0");
+            }
+            this.translogSizeInBytes = size;
+            return this;
+        }
+
+        public Builder uncommittedOperations(int operations) {
+            if (operations < 0) {
+                throw new IllegalArgumentException("uncommittedOperations must be >= 0");
+            }
+            this.uncommittedOperations = operations;
+            return this;
+        }
+
+        public Builder uncommittedSizeInBytes(long bytes) {
+            if (bytes < 0) {
+                throw new IllegalArgumentException("uncommittedSizeInBytes must be >= 0");
+            }
+            this.uncommittedSizeInBytes = bytes;
+            return this;
+        }
+
+        public Builder earliestLastModifiedAge(long age) {
+            if (age < 0) {
+                throw new IllegalArgumentException("earliestLastModifiedAge must be >= 0");
+            }
+            this.earliestLastModifiedAge = age;
+            return this;
+        }
+
+        /**
+         * Creates a {@link TranslogStats} object from the builder's current state.
+         * @return A new TranslogStats instance.
+         */
+        public TranslogStats build() {
+            return new TranslogStats(this);
+        }
+    }
+
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject(TRANSLOG);
diff --git a/server/src/test/java/org/opensearch/index/autoforcemerge/AutoForceMergeManagerTests.java b/server/src/test/java/org/opensearch/index/autoforcemerge/AutoForceMergeManagerTests.java
@@ -398,7 +398,12 @@ public void testShardValidatorWithValidShard() {
             getNodeWithRoles(DATA_NODE_1, Set.of(DiscoveryNodeRole.DATA_ROLE))
         );
         autoForceMergeManager.start();
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(6).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(6).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, 2);
         assertTrue(autoForceMergeManager.getShardValidator().validate(shard).isAllowed());
         autoForceMergeManager.close();
@@ -410,7 +415,12 @@ public void testShardValidatorWithShardNotInStartedState() {
             getNodeWithRoles(DATA_NODE_1, Set.of(DiscoveryNodeRole.DATA_ROLE))
         );
         autoForceMergeManager.start();
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(6).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(6).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, 2);
         when(shard.state()).thenReturn(IndexShardState.RECOVERING);
         assertFalse(autoForceMergeManager.getShardValidator().validate(shard).isAllowed());
@@ -437,7 +447,12 @@ public void testShardValidatorWithLowSegmentCount() {
             getNodeWithRoles(DATA_NODE_1, Set.of(DiscoveryNodeRole.DATA_ROLE))
         );
         autoForceMergeManager.start();
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(5).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(5).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, 1);
         assertFalse(autoForceMergeManager.getShardValidator().validate(shard).isAllowed());
         autoForceMergeManager.close();
@@ -451,7 +466,12 @@ public void testShardValidatorWithRecentTranslog() {
             getNodeWithRoles(DATA_NODE_1, Set.of(DiscoveryNodeRole.DATA_ROLE))
         );
         autoForceMergeManager.start();
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(1).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(1).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, 2);
         assertFalse(autoForceMergeManager.getShardValidator().validate(shard).isAllowed());
         autoForceMergeManager.close();
@@ -532,7 +552,12 @@ public void testForceMergeOperationOnDataNodeWithFailingMerges() throws IOExcept
         when(threadPool.stats()).thenReturn(stats);
 
         IndexService indexService1 = mock(IndexService.class);
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(6).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(6).getMillis())
+            .build();
         IndexShard shard1 = getShard(TEST_INDEX_1, translogStats, 2);
         List<IndexShard> indexShards1 = List.of(shard1);
         when(indexService1.spliterator()).thenReturn(indexShards1.spliterator());
@@ -592,7 +617,12 @@ public void testForceMergeOperationOnDataNodeOfWarmEnabledCluster() throws IOExc
         );
         when(threadPool.stats()).thenReturn(stats);
         IndexService indexService1 = mock(IndexService.class);
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(1).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(1).getMillis())
+            .build();
         IndexShard shard1 = getShard(TEST_INDEX_1, translogStats, 2);
         List<IndexShard> indexShards1 = List.of(shard1);
         when(indexService1.spliterator()).thenReturn(indexShards1.spliterator());
@@ -660,7 +690,12 @@ public void testForceMergeOperationOnDataNodeWithThreadInterruption() throws Int
         when(threadPool.stats()).thenReturn(stats);
 
         IndexService indexService1 = mock(IndexService.class);
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(1).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(1).getMillis())
+            .build();
         List<IndexShard> indexShards1 = List.of(getShard(TEST_INDEX_1, translogStats, 2));
         when(indexService1.spliterator()).thenReturn(indexShards1.spliterator());
         List<IndexService> indexServices = List.of(indexService1);
@@ -727,7 +762,12 @@ public void testMergesFailedMetric() throws IOException {
         setupIndexServiceWithShard(1024L, 2);
 
         IndexService indexService = mock(IndexService.class);
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(6).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(6).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, 2);
         doThrow(new IOException("Test exception")).when(shard).forceMerge(any());
 
@@ -851,7 +891,12 @@ private ExecutorService setupCompleteEnvironmentForMerge() {
 
     private void setupIndexServiceWithShard(long shardSize, int segmentCount) throws IOException {
         IndexService indexService = mock(IndexService.class);
-        TranslogStats translogStats = new TranslogStats(0, 0, 0, 0, TimeValue.timeValueSeconds(6).getMillis());
+        TranslogStats translogStats = new TranslogStats.Builder().numberOfOperations(0)
+            .translogSizeInBytes(0)
+            .uncommittedOperations(0)
+            .uncommittedSizeInBytes(0)
+            .earliestLastModifiedAge(TimeValue.timeValueSeconds(6).getMillis())
+            .build();
         IndexShard shard = getShard(TEST_INDEX_1, translogStats, segmentCount);
 
         if (shardSize != 1024L) {
diff --git a/server/src/test/java/org/opensearch/index/translog/LocalTranslogTests.java b/server/src/test/java/org/opensearch/index/translog/LocalTranslogTests.java
