From a631bb2dce9231cc3b1112f7659c5655d880c5a0 Mon Sep 17 00:00:00 2001 From: Karen X Date: Tue, 17 Feb 2026 02:57:17 +0000 Subject: [PATCH 1/2] [GRPC] Handle ShardSearchFailure properly Signed-off-by: Karen X --- CHANGELOG.md | 1 + gradle/libs.versions.toml | 2 +- .../licenses/protobufs-1.2.0.jar.sha1 | 1 - .../licenses/protobufs-1.3.0.jar.sha1 | 1 + .../spi/licenses/protobufs-1.2.0.jar.sha1 | 1 - .../spi/licenses/protobufs-1.3.0.jar.sha1 | 1 + ...ardOperationFailedExceptionProtoUtils.java | 7 +- .../ShardSearchFailureProtoUtils.java | 29 +- .../search/ShardStatisticsProtoUtils.java | 8 + ...erationFailedExceptionProtoUtilsTests.java | 6 +- .../ShardSearchFailureProtoUtilsTests.java | 113 ++++++++ .../ShardStatisticsProtoUtilsTests.java | 271 ++++++++++++++++++ 12 files changed, 431 insertions(+), 10 deletions(-) delete mode 100644 modules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1 create mode 100644 modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1 delete mode 100644 modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1 create mode 100644 modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1 create mode 100644 modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java create mode 100644 modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 8587f69d71ff1..772b8f9c4d35a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Service does not start on Windows with OpenJDK ([#20615](https://github.com/opensearch-project/OpenSearch/pull/20615)) - Update RemoteClusterStateCleanupManager to performed batched deletions of stale ClusterMetadataManifests and address deletion timeout issues ([#20566](https://github.com/opensearch-project/OpenSearch/pull/20566)) - Fix the regression of terms agg optimization at high cardinality ([#20623](https://github.com/opensearch-project/OpenSearch/pull/20623)) +- Fix ShardSearchFailure in transport-grpc ([#20641](https://github.com/opensearch-project/OpenSearch/pull/20641)) ### Dependencies - Bump `ch.qos.logback:logback-core` and `ch.qos.logback:logback-classic` from 1.5.24 to 1.5.27 ([#20525](https://github.com/opensearch-project/OpenSearch/pull/20525)) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 707310853a3bb..3a5b696e4305c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -24,7 +24,7 @@ kotlin = "1.7.10" antlr4 = "4.13.1" guava = "33.2.1-jre" gson = "2.13.2" -opensearchprotobufs = "1.2.0" +opensearchprotobufs = "1.3.0" protobuf = "3.25.8" jakarta_annotation = "1.3.5" google_http_client = "1.44.1" diff --git a/modules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1 b/modules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1 deleted file mode 100644 index b122dd4dca041..0000000000000 --- a/modules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -6688b30c1b5a73273e1062a6017b8de9f3ff7f33 \ No newline at end of file diff --git a/modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1 b/modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1 new file mode 100644 index 0000000000000..08f706404d3c8 --- /dev/null +++ b/modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1 @@ -0,0 +1 @@ +a141d00a9de80085436c648502d1b015fd89b9f6 \ No newline at end of file diff --git a/modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1 b/modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1 deleted file mode 100644 index b122dd4dca041..0000000000000 --- a/modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -6688b30c1b5a73273e1062a6017b8de9f3ff7f33 \ No newline at end of file diff --git a/modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1 b/modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1 new file mode 100644 index 0000000000000..08f706404d3c8 --- /dev/null +++ b/modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1 @@ -0,0 +1 @@ +a141d00a9de80085436c648502d1b015fd89b9f6 \ No newline at end of file diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java index 37c035ee3af39..c88193c201c8a 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java @@ -31,12 +31,15 @@ private ShardOperationFailedExceptionProtoUtils() { * This method is similar to {@link org.opensearch.core.action.ShardOperationFailedException#toXContent(XContentBuilder, ToXContent.Params)} * This method is overridden by various exception classes, which are hardcoded here. * + * This method converts to the legacy ShardFailure proto type for backward compatibility. + * For ShardSearchFailure, use the new failures_2 field with ShardSearchFailure proto type. + * * @param exception The ShardOperationFailedException to convert metadata from - * @return ShardFailure + * @return ShardFailure proto object */ public static ShardFailure toProto(ShardOperationFailedException exception) throws IOException { return switch (exception) { - case ShardSearchFailure ssf -> ShardSearchFailureProtoUtils.toProto(ssf); + case ShardSearchFailure ssf -> ShardSearchFailureProtoUtils.toLegacyProto(ssf); case SnapshotShardFailure ssf -> SnapshotShardFailureProtoUtils.toProto(ssf); case DefaultShardOperationFailedException dsofe -> DefaultShardOperationFailedExceptionProtoUtils.toProto(dsofe); case ReplicationResponse.ShardInfo.Failure sf -> ReplicationResponseShardInfoFailureProtoUtils.toProto(sf); diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java index 4d8c4555f7389..028a6f47fec64 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java @@ -25,13 +25,33 @@ private ShardSearchFailureProtoUtils() { } /** - * Converts the metadata from a ShardSearchFailure to a Protocol Buffer Struct. - * Similar to {@link ShardSearchFailure#toXContent(XContentBuilder, ToXContent.Params)} * + * Converts the metadata from a ShardSearchFailure to a Protocol Buffer ShardSearchFailure. + * Similar to {@link ShardSearchFailure#toXContent(XContentBuilder, ToXContent.Params)} * * @param exception The ShardSearchFailure to convert - * @return A Protocol Buffer Struct containing the exception metadata + * @return A Protocol Buffer ShardSearchFailure containing the exception metadata */ - public static ShardFailure toProto(ShardSearchFailure exception) throws IOException { + public static org.opensearch.protobufs.ShardSearchFailure toProto(ShardSearchFailure exception) throws IOException { + org.opensearch.protobufs.ShardSearchFailure.Builder shardSearchFailure = org.opensearch.protobufs.ShardSearchFailure.newBuilder(); + shardSearchFailure.setShard(exception.shardId()); + shardSearchFailure.setIndex(exception.index()); + if (exception.shard() != null && exception.shard().getNodeId() != null) { + shardSearchFailure.setNode(exception.shard().getNodeId()); + } + shardSearchFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause())); + return shardSearchFailure.build(); + } + + /** + * Converts the metadata from a ShardSearchFailure to the legacy ShardFailure proto type. + * This is for backward compatibility with older clients. + * @deprecated Use {@link #toProto(ShardSearchFailure)} which returns ShardSearchFailure proto type + * + * @param exception The ShardSearchFailure to convert + * @return A Protocol Buffer ShardFailure containing the exception metadata (without primary field) + */ + @Deprecated + public static ShardFailure toLegacyProto(ShardSearchFailure exception) throws IOException { ShardFailure.Builder shardFailure = ShardFailure.newBuilder(); shardFailure.setShard(exception.shardId()); shardFailure.setIndex(exception.index()); @@ -39,6 +59,7 @@ public static ShardFailure toProto(ShardSearchFailure exception) throws IOExcept shardFailure.setNode(exception.shard().getNodeId()); } shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause())); + // Note: primary field is not set for search failures as it's not applicable return shardFailure.build(); } } diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java index 05aad90870ad4..c86350e13c2b6 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java @@ -9,12 +9,14 @@ import org.opensearch.ExceptionsHelper; import org.opensearch.action.admin.indices.stats.ShardStats; +import org.opensearch.action.search.ShardSearchFailure; import org.opensearch.core.action.ShardOperationFailedException; import org.opensearch.core.common.util.CollectionUtils; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.protobufs.ShardStatistics; import org.opensearch.transport.grpc.proto.response.exceptions.shardoperationfailedexception.ShardOperationFailedExceptionProtoUtils; +import org.opensearch.transport.grpc.proto.response.exceptions.shardoperationfailedexception.ShardSearchFailureProtoUtils; import java.io.IOException; @@ -57,6 +59,12 @@ protected static ShardStatistics getShardStats( shardStats.setFailed(failed); if (CollectionUtils.isEmpty(shardFailures) == false) { for (ShardOperationFailedException shardFailure : ExceptionsHelper.groupBy(shardFailures)) { + // Populate the new failures_2 field with ShardSearchFailure proto type + if (shardFailure instanceof ShardSearchFailure) { + shardStats.addFailures2(ShardSearchFailureProtoUtils.toProto((ShardSearchFailure) shardFailure)); + } + + // Also populate the deprecated failures field for backward compatibility shardStats.addFailures(ShardOperationFailedExceptionProtoUtils.toProto(shardFailure)); } } diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java index 42c51d1916b75..1de0a03792555 100644 --- a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java @@ -34,7 +34,7 @@ public void testToProtoWithShardSearchFailure() throws IOException { // Create a ShardSearchFailure ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("fake exception"), searchShardTarget); - // Call the method under test + // Call the method under test - should call toLegacyProto for backward compatibility ShardFailure protoFailure = ShardOperationFailedExceptionProtoUtils.toProto(shardSearchFailure); // Verify the result @@ -42,6 +42,10 @@ public void testToProtoWithShardSearchFailure() throws IOException { assertEquals("Index should match", "test_index", protoFailure.getIndex()); assertEquals("Shard ID should match", 1, protoFailure.getShard()); assertEquals("Node ID should match", "test_node", protoFailure.getNode()); + + // Verify primary field is false (default) for search failures + // ShardSearchFailure doesn't set primary, so it defaults to false in the legacy proto + assertFalse("Primary should be false for search failures", protoFailure.getPrimary()); } public void testToProtoWithSnapshotShardFailure() throws IOException { diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java new file mode 100644 index 0000000000000..1bf1423382d01 --- /dev/null +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java @@ -0,0 +1,113 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.transport.grpc.proto.response.exceptions.shardoperationfailedexception; + +import org.opensearch.action.search.ShardSearchFailure; +import org.opensearch.core.index.shard.ShardId; +import org.opensearch.protobufs.ShardFailure; +import org.opensearch.search.SearchShardTarget; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; + +public class ShardSearchFailureProtoUtilsTests extends OpenSearchTestCase { + + public void testToProtoWithNodeId() throws IOException { + // Create a SearchShardTarget with a nodeId + ShardId shardId = new ShardId("test_index", "_na_", 1); + SearchShardTarget searchShardTarget = new SearchShardTarget("test_node", shardId, null, null); + + // Create a ShardSearchFailure + ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("fake exception"), searchShardTarget); + + // Call the method under test + org.opensearch.protobufs.ShardSearchFailure protoFailure = ShardSearchFailureProtoUtils.toProto(shardSearchFailure); + + // Verify the result + assertNotNull("Proto failure should not be null", protoFailure); + assertEquals("Index should match", "test_index", protoFailure.getIndex()); + assertEquals("Shard ID should match", 1, protoFailure.getShard()); + assertEquals("Node ID should match", "test_node", protoFailure.getNode()); + assertTrue("Reason should be set", protoFailure.hasReason()); + + // Verify primary field is NOT present (ShardSearchFailure proto doesn't have it) + // This is verified by the proto type itself - ShardSearchFailure doesn't define primary field + } + + public void testToProtoWithoutNodeId() throws IOException { + // Create a ShardSearchFailure without nodeId (shard target is null) + ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("fake exception")); + + // Call the method under test + org.opensearch.protobufs.ShardSearchFailure protoFailure = ShardSearchFailureProtoUtils.toProto(shardSearchFailure); + + // Verify the result + assertNotNull("Proto failure should not be null", protoFailure); + assertTrue("Reason should be set", protoFailure.hasReason()); + // Node should not be set when shard target is null + assertFalse("Node should not be set", protoFailure.hasNode()); + } + + public void testToLegacyProtoWithNodeId() throws IOException { + // Create a SearchShardTarget with a nodeId + ShardId shardId = new ShardId("legacy_index", "_na_", 2); + SearchShardTarget searchShardTarget = new SearchShardTarget("legacy_node", shardId, null, null); + + // Create a ShardSearchFailure + ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("legacy exception"), searchShardTarget); + + // Call the legacy method + ShardFailure protoFailure = ShardSearchFailureProtoUtils.toLegacyProto(shardSearchFailure); + + // Verify the result + assertNotNull("Proto failure should not be null", protoFailure); + assertEquals("Index should match", "legacy_index", protoFailure.getIndex()); + assertEquals("Shard ID should match", 2, protoFailure.getShard()); + assertEquals("Node ID should match", "legacy_node", protoFailure.getNode()); + assertTrue("Reason should be set", protoFailure.hasReason()); + + // Note: primary field exists in ShardFailure proto but is not set for search failures + // It will default to false + assertFalse("Primary should be false (default) for search failures", protoFailure.getPrimary()); + } + + public void testToLegacyProtoWithoutNodeId() throws IOException { + // Create a ShardSearchFailure without nodeId + ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("legacy exception without node")); + + // Call the legacy method + ShardFailure protoFailure = ShardSearchFailureProtoUtils.toLegacyProto(shardSearchFailure); + + // Verify the result + assertNotNull("Proto failure should not be null", protoFailure); + assertTrue("Reason should be set", protoFailure.hasReason()); + // Node should not be set when shard target is null + assertFalse("Node should not be set", protoFailure.hasNode()); + } + + public void testToProtoVsLegacyProtoHaveSameData() throws IOException { + // Create test data + ShardId shardId = new ShardId("compare_index", "_na_", 3); + SearchShardTarget searchShardTarget = new SearchShardTarget("compare_node", shardId, null, null); + ShardSearchFailure shardSearchFailure = new ShardSearchFailure(new Exception("compare exception"), searchShardTarget); + + // Get both proto types + org.opensearch.protobufs.ShardSearchFailure newProto = ShardSearchFailureProtoUtils.toProto(shardSearchFailure); + ShardFailure legacyProto = ShardSearchFailureProtoUtils.toLegacyProto(shardSearchFailure); + + // Verify common fields have same values + assertEquals("Index should match between new and legacy", legacyProto.getIndex(), newProto.getIndex()); + assertEquals("Shard should match between new and legacy", legacyProto.getShard(), newProto.getShard()); + assertEquals("Node should match between new and legacy", legacyProto.getNode(), newProto.getNode()); + + // Both should have reason set + assertTrue("New proto should have reason", newProto.hasReason()); + assertTrue("Legacy proto should have reason", legacyProto.hasReason()); + } +} diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java new file mode 100644 index 0000000000000..dcf9247460a4e --- /dev/null +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java @@ -0,0 +1,271 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.transport.grpc.proto.response.search; + +import org.opensearch.action.search.ShardSearchFailure; +import org.opensearch.core.action.ShardOperationFailedException; +import org.opensearch.core.index.shard.ShardId; +import org.opensearch.protobufs.ShardStatistics; +import org.opensearch.search.SearchShardTarget; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; + +public class ShardStatisticsProtoUtilsTests extends OpenSearchTestCase { + + public void testGetShardStatsBasicFields() throws IOException { + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 8, 1, 1, new ShardOperationFailedException[0]); + + assertEquals("Total shards should match", 10, stats.getTotal()); + assertEquals("Successful shards should match", 8, stats.getSuccessful()); + assertEquals("Skipped shards should match", 1, stats.getSkipped()); + assertEquals("Failed shards should match", 1, stats.getFailed()); + } + + public void testGetShardStatsWithNoSkipped() throws IOException { + // When skipped is negative, it should not be set + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 9, -1, 1, new ShardOperationFailedException[0]); + + assertEquals("Total shards should match", 10, stats.getTotal()); + assertEquals("Successful shards should match", 9, stats.getSuccessful()); + assertFalse("Skipped should not be set when negative", stats.hasSkipped()); + assertEquals("Failed shards should match", 1, stats.getFailed()); + } + + public void testGetShardStatsWithNoFailures() throws IOException { + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 10, 0, 0, new ShardOperationFailedException[0]); + + assertEquals("Total shards should match", 10, stats.getTotal()); + assertEquals("Successful shards should match", 10, stats.getSuccessful()); + assertEquals("Failed shards should be 0", 0, stats.getFailed()); + assertEquals("Failures list should be empty", 0, stats.getFailuresCount()); + assertEquals("Failures2 list should be empty", 0, stats.getFailures2Count()); + } + + public void testGetShardStatsWithNullFailures() throws IOException { + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 9, 0, 1, null); + + assertEquals("Total shards should match", 10, stats.getTotal()); + assertEquals("Successful shards should match", 9, stats.getSuccessful()); + assertEquals("Failed shards should match", 1, stats.getFailed()); + assertEquals("Failures list should be empty when null", 0, stats.getFailuresCount()); + assertEquals("Failures2 list should be empty when null", 0, stats.getFailures2Count()); + } + + public void testGetShardStatsWithShardSearchFailures() throws IOException { + // Create ShardSearchFailure instances + ShardId shardId1 = new ShardId("test_index", "_na_", 1); + SearchShardTarget target1 = new SearchShardTarget("node1", shardId1, null, null); + ShardSearchFailure failure1 = new ShardSearchFailure(new Exception("failure 1"), target1); + + ShardId shardId2 = new ShardId("test_index", "_na_", 2); + SearchShardTarget target2 = new SearchShardTarget("node2", shardId2, null, null); + ShardSearchFailure failure2 = new ShardSearchFailure(new Exception("failure 2"), target2); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure1, failure2 }; + + // Get stats + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 8, 0, 2, failures); + + assertEquals("Total shards should match", 10, stats.getTotal()); + assertEquals("Successful shards should match", 8, stats.getSuccessful()); + assertEquals("Failed shards should match", 2, stats.getFailed()); + + // Verify both the legacy failures field and new failures_2 field are populated + assertTrue("Legacy failures field should have entries", stats.getFailuresCount() > 0); + assertTrue("New failures_2 field should have entries", stats.getFailures2Count() > 0); + + // For ShardSearchFailure, both should have the same count + assertEquals( + "Both failures fields should have same count for search failures", + stats.getFailuresCount(), + stats.getFailures2Count() + ); + } + + public void testGetShardStatsPopulatesBothFieldsForBackwardCompatibility() throws IOException { + // Create a ShardSearchFailure + ShardId shardId = new ShardId("compat_index", "_na_", 3); + SearchShardTarget target = new SearchShardTarget("compat_node", shardId, null, null); + ShardSearchFailure failure = new ShardSearchFailure(new Exception("compat failure"), target); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure }; + + // Get stats + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(5, 4, 0, 1, failures); + + // Verify both fields are populated + assertEquals("Legacy failures field should have 1 entry", 1, stats.getFailuresCount()); + assertEquals("New failures_2 field should have 1 entry", 1, stats.getFailures2Count()); + + // Verify the legacy field contains a ShardFailure proto + assertTrue("Legacy failures should have at least one entry", stats.getFailuresList().size() > 0); + org.opensearch.protobufs.ShardFailure legacyFailure = stats.getFailures(0); + assertNotNull("Legacy failure should not be null", legacyFailure); + assertEquals("Legacy failure index should match", "compat_index", legacyFailure.getIndex()); + assertEquals("Legacy failure shard should match", 3, legacyFailure.getShard()); + + // Verify the new field contains a ShardSearchFailure proto + assertTrue("New failures_2 should have at least one entry", stats.getFailures2List().size() > 0); + org.opensearch.protobufs.ShardSearchFailure newFailure = stats.getFailures2(0); + assertNotNull("New failure should not be null", newFailure); + assertEquals("New failure index should match", "compat_index", newFailure.getIndex()); + assertEquals("New failure shard should match", 3, newFailure.getShard()); + + // Verify common fields match between old and new + assertEquals("Index should match between old and new", legacyFailure.getIndex(), newFailure.getIndex()); + assertEquals("Shard should match between old and new", legacyFailure.getShard(), newFailure.getShard()); + assertEquals("Node should match between old and new", legacyFailure.getNode(), newFailure.getNode()); + } + + public void testGetShardStatsWithMixedFailureTypes() throws IOException { + // Create a ShardSearchFailure (will populate both fields) + ShardId shardId1 = new ShardId("mixed_index", "_na_", 1); + SearchShardTarget target1 = new SearchShardTarget("node1", shardId1, null, null); + ShardSearchFailure searchFailure = new ShardSearchFailure(new Exception("search failure"), target1); + + // For this test, we're only testing ShardSearchFailure since that's what populates failures_2 + // Other failure types would only populate the legacy failures field + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { searchFailure }; + + // Get stats + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(5, 4, 0, 1, failures); + + // The legacy field should have all failures + assertEquals("Legacy failures field should have 1 entry", 1, stats.getFailuresCount()); + + // The new failures_2 field should only have ShardSearchFailure instances + assertEquals("New failures_2 field should have 1 entry (only ShardSearchFailure)", 1, stats.getFailures2Count()); + } + + /** + * Integration test: Verifies that both old and new fields are populated + * with multiple shard search failures for backward compatibility. + */ + public void testDualFieldPopulationWithMultipleFailures() throws IOException { + // Setup: Create multiple shard search failures + ShardId shard1 = new ShardId("index1", "_na_", 0); + ShardId shard2 = new ShardId("index2", "_na_", 1); + ShardId shard3 = new ShardId("index3", "_na_", 2); + + SearchShardTarget target1 = new SearchShardTarget("node1", shard1, null, null); + SearchShardTarget target2 = new SearchShardTarget("node2", shard2, null, null); + SearchShardTarget target3 = new SearchShardTarget("node3", shard3, null, null); + + ShardSearchFailure failure1 = new ShardSearchFailure(new RuntimeException("Timeout on shard 0"), target1); + ShardSearchFailure failure2 = new ShardSearchFailure(new RuntimeException("OOM on shard 1"), target2); + ShardSearchFailure failure3 = new ShardSearchFailure(new RuntimeException("Circuit breaker on shard 2"), target3); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure1, failure2, failure3 }; + + // Execute: Convert to proto + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(10, 7, 0, 3, failures); + + // Verify: Both fields should be populated + assertEquals("Should have 3 failures in legacy field", 3, stats.getFailuresCount()); + assertEquals("Should have 3 failures in new field", 3, stats.getFailures2Count()); + + // Verify: Data consistency between old and new fields + for (int i = 0; i < stats.getFailuresCount(); i++) { + org.opensearch.protobufs.ShardFailure legacyFailure = stats.getFailures(i); + org.opensearch.protobufs.ShardSearchFailure newFailure = stats.getFailures2(i); + + assertNotNull("Legacy failure " + i + " should not be null", legacyFailure); + assertNotNull("New failure " + i + " should not be null", newFailure); + + assertEquals("Index should match between legacy and new at position " + i, legacyFailure.getIndex(), newFailure.getIndex()); + assertEquals("Shard should match between legacy and new at position " + i, legacyFailure.getShard(), newFailure.getShard()); + assertEquals("Node should match between legacy and new at position " + i, legacyFailure.getNode(), newFailure.getNode()); + } + } + + /** + * Integration test: Simulates old client reading from deprecated 'failures' field. + */ + public void testOldClientCanReadLegacyField() throws IOException { + ShardId shardId = new ShardId("old_client_test", "_na_", 5); + SearchShardTarget target = new SearchShardTarget("old_node", shardId, null, null); + ShardSearchFailure failure = new ShardSearchFailure(new Exception("old client error"), target); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure }; + + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(1, 0, 0, 1, failures); + + // Old client reads from 'failures' field + assertTrue("Old client should see failures in legacy field", stats.getFailuresCount() > 0); + org.opensearch.protobufs.ShardFailure legacyFailure = stats.getFailures(0); + + // Old client can extract all necessary information + assertEquals("Old client can read index", "old_client_test", legacyFailure.getIndex()); + assertEquals("Old client can read shard", 5, legacyFailure.getShard()); + assertEquals("Old client can read node", "old_node", legacyFailure.getNode()); + assertTrue("Old client can read reason", legacyFailure.hasReason()); + + // Note: Old client will see primary=false (default), which is acceptable for search failures + assertFalse("Primary defaults to false for search failures", legacyFailure.getPrimary()); + } + + /** + * Integration test: Simulates new client reading from 'failures_2' field + * which uses ShardSearchFailure proto type (without primary field). + */ + public void testNewClientCanReadNewField() throws IOException { + ShardId shardId = new ShardId("new_client_test", "_na_", 7); + SearchShardTarget target = new SearchShardTarget("new_node", shardId, null, null); + ShardSearchFailure failure = new ShardSearchFailure(new Exception("new client error"), target); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure }; + + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(1, 0, 0, 1, failures); + + // New client reads from 'failures_2' field + assertTrue("New client should see failures in new field", stats.getFailures2Count() > 0); + org.opensearch.protobufs.ShardSearchFailure newFailure = stats.getFailures2(0); + + // New client can extract all necessary information + assertEquals("New client can read index", "new_client_test", newFailure.getIndex()); + assertEquals("New client can read shard", 7, newFailure.getShard()); + assertEquals("New client can read node", "new_node", newFailure.getNode()); + assertTrue("New client can read reason", newFailure.hasReason()); + + // New client benefits from not having the 'primary' field in the proto definition + // so there's no confusion about whether it's required or what it means for search failures + } + + /** + * Integration test: Demonstrates gradual migration support where both + * old and new clients can work simultaneously during the transition period. + */ + public void testGradualMigrationSupport() throws IOException { + // During migration: Some clients use old proto, some use new proto + ShardId shardId = new ShardId("migration_test", "_na_", 9); + SearchShardTarget target = new SearchShardTarget("migration_node", shardId, null, null); + ShardSearchFailure failure = new ShardSearchFailure(new Exception("migration error"), target); + + ShardOperationFailedException[] failures = new ShardOperationFailedException[] { failure }; + + ShardStatistics stats = ShardStatisticsProtoUtils.getShardStats(2, 1, 0, 1, failures); + + // Both fields are populated + assertTrue("Legacy field should be populated during migration", stats.getFailuresCount() > 0); + assertTrue("New field should be populated during migration", stats.getFailures2Count() > 0); + + // Old clients continue working (read from failures) + org.opensearch.protobufs.ShardFailure legacyFailure = stats.getFailures(0); + assertNotNull("Old clients can still deserialize", legacyFailure); + + // New clients can start using new field (read from failures_2) + org.opensearch.protobufs.ShardSearchFailure newFailure = stats.getFailures2(0); + assertNotNull("New clients can deserialize new field", newFailure); + + // Both get correct data + assertEquals("Both clients see same index", legacyFailure.getIndex(), newFailure.getIndex()); + assertEquals("Both clients see same shard", legacyFailure.getShard(), newFailure.getShard()); + } +} From 79c8f064cf065705fc4f137a170c16c323a8c825 Mon Sep 17 00:00:00 2001 From: Karen X Date: Tue, 17 Feb 2026 17:53:00 +0000 Subject: [PATCH 2/2] null check Signed-off-by: Karen X --- .../ShardSearchFailureProtoUtils.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java index 028a6f47fec64..e28062ec15536 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java @@ -34,7 +34,9 @@ private ShardSearchFailureProtoUtils() { public static org.opensearch.protobufs.ShardSearchFailure toProto(ShardSearchFailure exception) throws IOException { org.opensearch.protobufs.ShardSearchFailure.Builder shardSearchFailure = org.opensearch.protobufs.ShardSearchFailure.newBuilder(); shardSearchFailure.setShard(exception.shardId()); - shardSearchFailure.setIndex(exception.index()); + if (exception.index() != null) { + shardSearchFailure.setIndex(exception.index()); + } if (exception.shard() != null && exception.shard().getNodeId() != null) { shardSearchFailure.setNode(exception.shard().getNodeId()); } @@ -54,7 +56,9 @@ public static org.opensearch.protobufs.ShardSearchFailure toProto(ShardSearchFai public static ShardFailure toLegacyProto(ShardSearchFailure exception) throws IOException { ShardFailure.Builder shardFailure = ShardFailure.newBuilder(); shardFailure.setShard(exception.shardId()); - shardFailure.setIndex(exception.index()); + if (exception.index() != null) { + shardFailure.setIndex(exception.index()); + } if (exception.shard() != null && exception.shard().getNodeId() != null) { shardFailure.setNode(exception.shard().getNodeId()); }