From ed359f0d380659c98d9b313fba9ba4d2d6025e35 Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Thu, 6 Oct 2022 11:08:17 -0700 Subject: [PATCH 1/3] Fix new race condition in DecommissionControllerTests (#4688) My previous fix introduced a new race condition by making the assertions before waiting on the latch. Signed-off-by: Andrew Ross Signed-off-by: Andrew Ross --- CHANGELOG.md | 1 + .../cluster/decommission/DecommissionControllerTests.java | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 868ce6ed7bf9e..a9efbaed8a671 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,6 +118,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Bug]: Alias filter lost after rollover ([#4499](https://github.com/opensearch-project/OpenSearch/pull/4499)) - Attempt to fix Github workflow for Gradle Check job ([#4679](https://github.com/opensearch-project/OpenSearch/pull/4679)) - Fix flaky DecommissionControllerTests.testTimesOut ([4683](https://github.com/opensearch-project/OpenSearch/pull/4683)) +- Fix new race condition in DecommissionControllerTests ([4688](https://github.com/opensearch-project/OpenSearch/pull/4688)) ### Security diff --git a/server/src/test/java/org/opensearch/cluster/decommission/DecommissionControllerTests.java b/server/src/test/java/org/opensearch/cluster/decommission/DecommissionControllerTests.java index 5b2dce277189c..06f2de04907d6 100644 --- a/server/src/test/java/org/opensearch/cluster/decommission/DecommissionControllerTests.java +++ b/server/src/test/java/org/opensearch/cluster/decommission/DecommissionControllerTests.java @@ -223,7 +223,9 @@ public void testTimesOut() throws InterruptedException { TimeValue.timeValueMillis(0), new ActionListener<>() { @Override - public void onResponse(Void unused) {} + public void onResponse(Void unused) { + countDownLatch.countDown(); + } @Override public void onFailure(Exception e) { @@ -232,10 +234,10 @@ public void onFailure(Exception e) { } } ); + assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); MatcherAssert.assertThat("Expected onFailure to be called", exceptionReference.get(), notNullValue()); MatcherAssert.assertThat(exceptionReference.get(), instanceOf(OpenSearchTimeoutException.class)); MatcherAssert.assertThat(exceptionReference.get().getMessage(), containsString("waiting for removal of decommissioned nodes")); - assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); } public void testSuccessfulDecommissionStatusMetadataUpdate() throws InterruptedException { From a2f95ce324d7869701bb8229a28f917af08214f8 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 6 Oct 2022 16:24:31 -0400 Subject: [PATCH 2/3] Fixing SearchStats (de)serialization (#4697) Fixes bwc failure caused by commit 6993ac9834d0f7e8e77e6cade09de2245d2a20f7. Signed-off-by: Andriy Redko --- CHANGELOG.md | 1 + .../rest-api-spec/test/cat.shards/10_basic.yml | 14 +++++++------- .../resources/rest-api-spec/test/pit/10_basic.yml | 4 ++-- .../opensearch/index/search/stats/SearchStats.java | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9efbaed8a671..b08c21d54be73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,6 +119,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Attempt to fix Github workflow for Gradle Check job ([#4679](https://github.com/opensearch-project/OpenSearch/pull/4679)) - Fix flaky DecommissionControllerTests.testTimesOut ([4683](https://github.com/opensearch-project/OpenSearch/pull/4683)) - Fix new race condition in DecommissionControllerTests ([4688](https://github.com/opensearch-project/OpenSearch/pull/4688)) +- Fix SearchStats (de)serialization (caused by https://github.com/opensearch-project/OpenSearch/pull/4616) ([#4697](https://github.com/opensearch-project/OpenSearch/pull/4697)) ### Security diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.shards/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.shards/10_basic.yml index 6ebe273d552cc..189215b6562a3 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.shards/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.shards/10_basic.yml @@ -1,14 +1,14 @@ --- "Help": - skip: - version: " - 2.9.99" - reason: point in time stats were added in 3.0.0 + version: " - 2.3.99" + reason: point in time stats were added in 2.4.0 features: node_selector - do: cat.shards: help: true node_selector: - version: "3.0.0 - " + version: "2.4.0 - " - match: $body: | @@ -88,16 +88,16 @@ path.state .+ \n $/ --- -"Help before - 3.0.0": +"Help before - 2.4.0": - skip: - version: "3.0.0 - " - reason: point in time stats were added in 3.0.0 + version: "2.4.0 - " + reason: point in time stats were added in 2.4.0 features: node_selector - do: cat.shards: help: true node_selector: - version: " - 2.9.99" + version: " - 2.3.99" - match: $body: | diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml index cd0c5b9126a9d..847131ead35b6 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml @@ -1,7 +1,7 @@ "Create PIT, Search with PIT ID and Delete": - skip: - version: " - 2.9.99" - reason: "mode to be introduced later than 3.0" + version: " - 2.3.99" + reason: "mode to be introduced later than 2.4.0" - do: indices.create: index: test_pit diff --git a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java index 38b18355fd98d..012c94639d526 100644 --- a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java @@ -141,7 +141,7 @@ private Stats(StreamInput in) throws IOException { suggestTimeInMillis = in.readVLong(); suggestCurrent = in.readVLong(); - if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + if (in.getVersion().onOrAfter(Version.V_2_4_0)) { pitCount = in.readVLong(); pitTimeInMillis = in.readVLong(); pitCurrent = in.readVLong(); @@ -292,7 +292,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeVLong(suggestTimeInMillis); out.writeVLong(suggestCurrent); - if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + if (out.getVersion().onOrAfter(Version.V_2_4_0)) { out.writeVLong(pitCount); out.writeVLong(pitTimeInMillis); out.writeVLong(pitCurrent); From f5ea1a4d8067896080f61ab5f88cdcd307ee3c84 Mon Sep 17 00:00:00 2001 From: Xue Zhou <85715413+xuezhou25@users.noreply.github.com> Date: Thu, 6 Oct 2022 19:46:07 -0700 Subject: [PATCH 3/3] Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled (#4669) * Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled Signed-off-by: Xue Zhou --- CHANGELOG.md | 1 + .../java/org/opensearch/ExceptionsHelper.java | 15 +++++++++++++++ .../java/org/opensearch/OpenSearchException.java | 4 +--- .../org/opensearch/ExceptionsHelperTests.java | 6 ++++++ .../org/opensearch/OpenSearchExceptionTests.java | 7 +------ .../opensearch/rest/BytesRestResponseTests.java | 2 +- 6 files changed, 25 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b08c21d54be73..f8bfaf5abfed3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Segment Replication] Adding check to make sure checkpoint is not processed when a shard's shard routing is primary ([#4630](https://github.com/opensearch-project/OpenSearch/pull/4630)) - [Bug]: Fixed invalid location of JDK dependency for arm64 architecture([#4613](https://github.com/opensearch-project/OpenSearch/pull/4613)) - [Bug]: Alias filter lost after rollover ([#4499](https://github.com/opensearch-project/OpenSearch/pull/4499)) +- Fixed misunderstanding message "No OpenSearchException found" when detailed_error disabled ([#4669](https://github.com/opensearch-project/OpenSearch/pull/4669)) - Attempt to fix Github workflow for Gradle Check job ([#4679](https://github.com/opensearch-project/OpenSearch/pull/4679)) - Fix flaky DecommissionControllerTests.testTimesOut ([4683](https://github.com/opensearch-project/OpenSearch/pull/4683)) - Fix new race condition in DecommissionControllerTests ([4688](https://github.com/opensearch-project/OpenSearch/pull/4688)) diff --git a/server/src/main/java/org/opensearch/ExceptionsHelper.java b/server/src/main/java/org/opensearch/ExceptionsHelper.java index f252d0b05af79..fbfc9beaea468 100644 --- a/server/src/main/java/org/opensearch/ExceptionsHelper.java +++ b/server/src/main/java/org/opensearch/ExceptionsHelper.java @@ -99,6 +99,21 @@ public static RestStatus status(Throwable t) { return RestStatus.INTERNAL_SERVER_ERROR; } + public static String summaryMessage(Throwable t) { + if (t != null) { + if (t instanceof OpenSearchException) { + return t.getClass().getSimpleName() + "[" + t.getMessage() + "]"; + } else if (t instanceof IllegalArgumentException) { + return "Invalid argument"; + } else if (t instanceof JsonParseException) { + return "Failed to parse JSON"; + } else if (t instanceof OpenSearchRejectedExecutionException) { + return "Too many requests"; + } + } + return "Internal failure"; + } + public static Throwable unwrapCause(Throwable t) { int counter = 0; Throwable result = t; diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index 34d7509c7afb2..4b6ca173ec692 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -594,16 +594,14 @@ public static void generateFailureXContent(XContentBuilder builder, Params param // Render the exception with a simple message if (detailed == false) { - String message = "No OpenSearchException found"; Throwable t = e; for (int counter = 0; counter < 10 && t != null; counter++) { if (t instanceof OpenSearchException) { - message = t.getClass().getSimpleName() + "[" + t.getMessage() + "]"; break; } t = t.getCause(); } - builder.field(ERROR, message); + builder.field(ERROR, ExceptionsHelper.summaryMessage(t)); return; } diff --git a/server/src/test/java/org/opensearch/ExceptionsHelperTests.java b/server/src/test/java/org/opensearch/ExceptionsHelperTests.java index d16b2e9d291b0..41051d7ff2dd0 100644 --- a/server/src/test/java/org/opensearch/ExceptionsHelperTests.java +++ b/server/src/test/java/org/opensearch/ExceptionsHelperTests.java @@ -113,6 +113,12 @@ public void testStatus() { assertThat(ExceptionsHelper.status(new OpenSearchRejectedExecutionException("rejected")), equalTo(RestStatus.TOO_MANY_REQUESTS)); } + public void testSummaryMessage() { + assertThat(ExceptionsHelper.summaryMessage(new IllegalArgumentException("illegal")), equalTo("Invalid argument")); + assertThat(ExceptionsHelper.summaryMessage(new JsonParseException(null, "illegal")), equalTo("Failed to parse JSON")); + assertThat(ExceptionsHelper.summaryMessage(new OpenSearchRejectedExecutionException("rejected")), equalTo("Too many requests")); + } + public void testGroupBy() { ShardOperationFailedException[] failures = new ShardOperationFailedException[] { createShardFailureParsingException("error", "node0", "index", 0, null), diff --git a/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java b/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java index bd2695508dfcb..6ceb1d6f12e3b 100644 --- a/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java +++ b/server/src/test/java/org/opensearch/OpenSearchExceptionTests.java @@ -814,12 +814,7 @@ public void testFailureToAndFromXContentWithNoDetails() throws IOException { } assertNotNull(parsedFailure); - String reason; - if (failure instanceof OpenSearchException) { - reason = failure.getClass().getSimpleName() + "[" + failure.getMessage() + "]"; - } else { - reason = "No OpenSearchException found"; - } + String reason = ExceptionsHelper.summaryMessage(failure); assertEquals(OpenSearchException.buildMessage("exception", reason, null), parsedFailure.getMessage()); assertEquals(0, parsedFailure.getHeaders().size()); assertEquals(0, parsedFailure.getMetadata().size()); diff --git a/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java b/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java index 1ea7f006cf482..20a41b1d8d120 100644 --- a/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java +++ b/server/src/test/java/org/opensearch/rest/BytesRestResponseTests.java @@ -117,7 +117,7 @@ public void testNonOpenSearchExceptionIsNotShownAsSimpleMessage() throws Excepti assertThat(text, not(containsString("UnknownException[an error occurred reading data]"))); assertThat(text, not(containsString("FileNotFoundException[/foo/bar]"))); assertThat(text, not(containsString("error_trace"))); - assertThat(text, containsString("\"error\":\"No OpenSearchException found\"")); + assertThat(text, containsString("\"error\":\"Internal failure\"")); } public void testErrorTrace() throws Exception {