diff --git a/CHANGELOG.md b/CHANGELOG.md index 0930537b72926..60247efa40948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792)) - Pass localNode info to all plugins on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) - Improved performance of parsing floating point numbers ([#7909](https://github.com/opensearch-project/OpenSearch/pull/7909)) +- Move span actions to Scope ([#8411](https://github.com/opensearch-project/OpenSearch/pull/8411)) ### Deprecated @@ -97,6 +98,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Enable Point based optimization for custom comparators ([#8168](https://github.com/opensearch-project/OpenSearch/pull/8168)) - [Extensions] Support extension additional settings with extension REST initialization ([#8414](https://github.com/opensearch-project/OpenSearch/pull/8414)) - Adds mock implementation for TelemetryPlugin ([#7545](https://github.com/opensearch-project/OpenSearch/issues/7545)) +- Support transport action names when registering NamedRoutes ([#7957](https://github.com/opensearch-project/OpenSearch/pull/7957)) ### Dependencies - Bump `com.azure:azure-storage-common` from 12.21.0 to 12.21.1 (#7566, #7814) @@ -131,6 +133,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Bump `com.google.api-client:google-api-client` from 1.34.0 to 2.2.0 ([#8276](https://github.com/opensearch-project/OpenSearch/pull/8276)) - Update Apache HttpCore/ HttpClient and Apache HttpCore5 / HttpClient5 dependencies ([#8434](https://github.com/opensearch-project/OpenSearch/pull/8434)) - Bump `org.apache.maven:maven-model` from 3.9.2 to 3.9.3 (#8403) +- Bump `io.projectreactor.netty:reactor-netty` and `io.projectreactor.netty:reactor-netty-core` from 1.1.7 to 1.1.8 (#8405) ### Changed - Replace jboss-annotations-api_1.2_spec with jakarta.annotation-api ([#7836](https://github.com/opensearch-project/OpenSearch/pull/7836)) @@ -149,6 +152,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Check UTF16 string size before converting to String to avoid OOME ([#7963](https://github.com/opensearch-project/OpenSearch/pull/7963)) - Move ZSTD compression codecs out of the sandbox ([#7908](https://github.com/opensearch-project/OpenSearch/pull/7908)) - Update Gradle check to modify an existing comment on PRs instead of creating a new one ([#8488](https://github.com/opensearch-project/OpenSearch/pull/8488)) +- Update ZSTD default compression level ([#8471](https://github.com/opensearch-project/OpenSearch/pull/8471)) ### Deprecated diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java new file mode 100644 index 0000000000000..58e9e0abad739 --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java @@ -0,0 +1,70 @@ +/* + * 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.telemetry.tracing; + +import java.util.function.Consumer; + +/** + * Default implementation of Scope + */ +public class DefaultSpanScope implements SpanScope { + + private final Span span; + + private final Consumer onCloseConsumer; + + /** + * Creates Scope instance for the given span + * + * @param span underlying span + * @param onCloseConsumer consumer to execute on scope close + */ + public DefaultSpanScope(Span span, Consumer onCloseConsumer) { + this.span = span; + this.onCloseConsumer = onCloseConsumer; + } + + @Override + public void addSpanAttribute(String key, String value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, long value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, double value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanAttribute(String key, boolean value) { + span.addAttribute(key, value); + } + + @Override + public void addSpanEvent(String event) { + span.addEvent(event); + } + + @Override + public void setError(Exception exception) { + span.setError(exception); + } + + /** + * Executes the runnable to end the scope + */ + @Override + public void close() { + onCloseConsumer.accept(span); + } +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java index ab9110af7c3ab..783edd238c1c2 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java @@ -13,8 +13,8 @@ /** * - * The default tracer implementation. This class implements the basic logic for span lifecycle and its state management. - * It also handles tracing context propagation between spans. + * The default tracer implementation. It handles tracing context propagation between spans by maintaining + * current active span in its storage * * */ @@ -36,41 +36,11 @@ public DefaultTracer(TracingTelemetry tracingTelemetry, TracerContextStorage endSpan(span)); - } - - @Override - public void addSpanAttribute(String key, String value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, long value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, double value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanAttribute(String key, boolean value) { - Span currentSpan = getCurrentSpan(); - currentSpan.addAttribute(key, value); - } - - @Override - public void addSpanEvent(String event) { - Span currentSpan = getCurrentSpan(); - currentSpan.addEvent(event); + return new DefaultSpanScope(span, (scopeSpan) -> endSpan(scopeSpan)); } @Override diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java deleted file mode 100644 index 52f4eaf648eea..0000000000000 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Scope.java +++ /dev/null @@ -1,26 +0,0 @@ -/* - * 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.telemetry.tracing; - -/** - * An auto-closeable that represents scope of the span. - * It is recommended that you use this class with a try-with-resources block: - */ -public interface Scope extends AutoCloseable { - /** - * No-op Scope implementation - */ - Scope NO_OP = () -> {}; - - /** - * closes the scope - */ - @Override - void close(); -} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java deleted file mode 100644 index 30a7ac7fa90e7..0000000000000 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/ScopeImpl.java +++ /dev/null @@ -1,33 +0,0 @@ -/* - * 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.telemetry.tracing; - -/** - * Executes the runnable on close - */ -public class ScopeImpl implements Scope { - - private Runnable runnableOnClose; - - /** - * Creates Scope instance - * @param runnableOnClose runnable to execute on scope close - */ - public ScopeImpl(Runnable runnableOnClose) { - this.runnableOnClose = runnableOnClose; - } - - /** - * Executes the runnable to end the scope - */ - @Override - public void close() { - runnableOnClose.run(); - } -} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java index 0710b8a22a37f..d60b4e60adece 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java @@ -62,6 +62,13 @@ public interface Span { */ void addAttribute(String key, Boolean value); + /** + * Records error in the span + * + * @param exception exception to be recorded + */ + void setError(Exception exception); + /** * Adds an event in the span * diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java new file mode 100644 index 0000000000000..cf67165d889bc --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanScope.java @@ -0,0 +1,74 @@ +/* + * 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.telemetry.tracing; + +import org.opensearch.telemetry.tracing.noop.NoopSpanScope; + +/** + * An auto-closeable that represents scope of the span. + * It provides interface for all the span operations. + */ +public interface SpanScope extends AutoCloseable { + /** + * No-op Scope implementation + */ + SpanScope NO_OP = new NoopSpanScope(); + + /** + * Adds string attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, String value); + + /** + * Adds long attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, long value); + + /** + * Adds double attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, double value); + + /** + * Adds boolean attribute to the {@link Span}. + * + * @param key attribute key + * @param value attribute value + */ + void addSpanAttribute(String key, boolean value); + + /** + * Adds an event to the {@link Span}. + * + * @param event event name + */ + void addSpanEvent(String event); + + /** + * Records error in the span + * + * @param exception exception to be recorded + */ + void setError(Exception exception); + + /** + * closes the scope + */ + @Override + void close(); +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java index fcc091eb39c48..d422b58aa0a9f 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java @@ -11,7 +11,7 @@ import java.io.Closeable; /** - * Tracer is the interface used to create a {@link Span} and interact with current active {@link Span}. + * Tracer is the interface used to create a {@link Span} * It automatically handles the context propagation between threads, tasks, nodes etc. * * All methods on the Tracer object are multi-thread safe. @@ -24,44 +24,6 @@ public interface Tracer extends Closeable { * @param spanName span name * @return scope of the span, must be closed with explicit close or with try-with-resource */ - Scope startSpan(String spanName); + SpanScope startSpan(String spanName); - /** - * Adds string attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, String value); - - /** - * Adds long attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, long value); - - /** - * Adds double attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, double value); - - /** - * Adds boolean attribute to the current active {@link Span}. - * - * @param key attribute key - * @param value attribute value - */ - void addSpanAttribute(String key, boolean value); - - /** - * Adds an event to the current active {@link Span}. - * - * @param event event name - */ - void addSpanEvent(String event); } diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java new file mode 100644 index 0000000000000..c0dbaf65ba48b --- /dev/null +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java @@ -0,0 +1,57 @@ +/* + * 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.telemetry.tracing.noop; + +import org.opensearch.telemetry.tracing.SpanScope; + +/** + * No-op implementation of SpanScope + */ +public final class NoopSpanScope implements SpanScope { + + /** + * No-args constructor + */ + public NoopSpanScope() {} + + @Override + public void addSpanAttribute(String key, String value) { + + } + + @Override + public void addSpanAttribute(String key, long value) { + + } + + @Override + public void addSpanAttribute(String key, double value) { + + } + + @Override + public void addSpanAttribute(String key, boolean value) { + + } + + @Override + public void addSpanEvent(String event) { + + } + + @Override + public void setError(Exception exception) { + + } + + @Override + public void close() { + + } +} diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java index 18fc60e41e54d..a66cbcf4fef52 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java @@ -8,7 +8,7 @@ package org.opensearch.telemetry.tracing.noop; -import org.opensearch.telemetry.tracing.Scope; +import org.opensearch.telemetry.tracing.SpanScope; import org.opensearch.telemetry.tracing.Tracer; /** @@ -24,49 +24,8 @@ public class NoopTracer implements Tracer { private NoopTracer() {} @Override - public Scope startSpan(String spanName) { - return Scope.NO_OP; - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, String value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, long value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, double value) { - - } - - /** - * @param key attribute key - * @param value attribute value - */ - @Override - public void addSpanAttribute(String key, boolean value) { - - } - - @Override - public void addSpanEvent(String event) { - + public SpanScope startSpan(String spanName) { + return SpanScope.NO_OP; } @Override diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java new file mode 100644 index 0000000000000..eea6b77ce6e1e --- /dev/null +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultSpanScopeTests.java @@ -0,0 +1,79 @@ +/* + * 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.telemetry.tracing; + +import org.opensearch.test.OpenSearchTestCase; + +import java.util.function.Consumer; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +public class DefaultSpanScopeTests extends OpenSearchTestCase { + + @SuppressWarnings("unchecked") + public void testClose() { + Span mockSpan = mock(Span.class); + Consumer mockConsumer = mock(Consumer.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, mockConsumer); + defaultSpanScope.close(); + + verify(mockConsumer).accept(mockSpan); + } + + public void testAddSpanAttributeString() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", "value"); + + verify(mockSpan).addAttribute("key", "value"); + } + + public void testAddSpanAttributeLong() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", 1L); + + verify(mockSpan).addAttribute("key", 1L); + } + + public void testAddSpanAttributeDouble() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", 1.0); + + verify(mockSpan).addAttribute("key", 1.0); + } + + public void testAddSpanAttributeBoolean() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanAttribute("key", true); + + verify(mockSpan).addAttribute("key", true); + } + + public void testAddEvent() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + defaultSpanScope.addSpanEvent("eventName"); + + verify(mockSpan).addEvent("eventName"); + } + + public void testSetError() { + Span mockSpan = mock(Span.class); + DefaultSpanScope defaultSpanScope = new DefaultSpanScope(mockSpan, null); + Exception ex = new Exception("error"); + defaultSpanScope.setError(ex); + + verify(mockSpan).setError(ex); + } + +} diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java index f0e8f3c2e2344..2b7a379b0051a 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java @@ -45,57 +45,12 @@ public void testCreateSpan() { public void testEndSpanByClosingScope() { DefaultTracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - try (Scope scope = defaultTracer.startSpan("span_name")) { + try (SpanScope spanScope = defaultTracer.startSpan("span_name")) { verify(mockTracerContextStorage).put(TracerContextStorage.CURRENT_SPAN, mockSpan); } verify(mockTracerContextStorage).put(TracerContextStorage.CURRENT_SPAN, mockParentSpan); } - public void testAddSpanAttributeString() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", "value"); - - verify(mockSpan).addAttribute("key", "value"); - } - - public void testAddSpanAttributeLong() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", 1L); - - verify(mockSpan).addAttribute("key", 1L); - } - - public void testAddSpanAttributeDouble() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", 1.0); - - verify(mockSpan).addAttribute("key", 1.0); - } - - public void testAddSpanAttributeBoolean() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanAttribute("key", true); - - verify(mockSpan).addAttribute("key", true); - } - - public void testAddEvent() { - Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); - defaultTracer.startSpan("span_name"); - - defaultTracer.addSpanEvent("eventName"); - - verify(mockSpan).addEvent("eventName"); - } - public void testClose() throws IOException { Tracer defaultTracer = new DefaultTracer(mockTracingTelemetry, mockTracerContextStorage); diff --git a/plugins/repository-azure/build.gradle b/plugins/repository-azure/build.gradle index e67ea7ab0a11e..4edb9e0b1913e 100644 --- a/plugins/repository-azure/build.gradle +++ b/plugins/repository-azure/build.gradle @@ -58,8 +58,8 @@ dependencies { api 'com.azure:azure-storage-blob:12.22.2' api 'org.reactivestreams:reactive-streams:1.0.4' api 'io.projectreactor:reactor-core:3.5.6' - api 'io.projectreactor.netty:reactor-netty:1.1.7' - api 'io.projectreactor.netty:reactor-netty-core:1.1.7' + api 'io.projectreactor.netty:reactor-netty:1.1.8' + api 'io.projectreactor.netty:reactor-netty-core:1.1.8' api 'io.projectreactor.netty:reactor-netty-http:1.1.8' api "org.slf4j:slf4j-api:${versions.slf4j}" api "com.fasterxml.jackson.core:jackson-annotations:${versions.jackson}" diff --git a/plugins/repository-azure/licenses/reactor-netty-1.1.7.jar.sha1 b/plugins/repository-azure/licenses/reactor-netty-1.1.7.jar.sha1 deleted file mode 100644 index 01a9b1d34d52f..0000000000000 --- a/plugins/repository-azure/licenses/reactor-netty-1.1.7.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -c16497c29f96ea7b1db538cb0ddde55d9be173fe \ No newline at end of file diff --git a/plugins/repository-azure/licenses/reactor-netty-1.1.8.jar.sha1 b/plugins/repository-azure/licenses/reactor-netty-1.1.8.jar.sha1 new file mode 100644 index 0000000000000..6b6bf1903b16c --- /dev/null +++ b/plugins/repository-azure/licenses/reactor-netty-1.1.8.jar.sha1 @@ -0,0 +1 @@ +d53a9d7d0395285f4c81664494fcd61477626e32 \ No newline at end of file diff --git a/plugins/repository-azure/licenses/reactor-netty-core-1.1.7.jar.sha1 b/plugins/repository-azure/licenses/reactor-netty-core-1.1.7.jar.sha1 deleted file mode 100644 index 62ed795cb11e9..0000000000000 --- a/plugins/repository-azure/licenses/reactor-netty-core-1.1.7.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -d38bb526a501f52c4476b03730c710a96f8fd35b \ No newline at end of file diff --git a/plugins/repository-azure/licenses/reactor-netty-core-1.1.8.jar.sha1 b/plugins/repository-azure/licenses/reactor-netty-core-1.1.8.jar.sha1 new file mode 100644 index 0000000000000..707631f4dfe0c --- /dev/null +++ b/plugins/repository-azure/licenses/reactor-netty-core-1.1.8.jar.sha1 @@ -0,0 +1 @@ +48999c4ae27cdcee5eaff9dfd150a8b64624f0f5 \ No newline at end of file diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java index 23a2d9baa3e6e..ba63df4ae47a1 100644 --- a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java +++ b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java @@ -9,6 +9,7 @@ package org.opensearch.telemetry.tracing; import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.StatusCode; /** * Default implementation of {@link Span} using Otel span. It keeps a reference of OpenTelemetry Span and handles span @@ -48,6 +49,11 @@ public void addAttribute(String key, Boolean value) { delegateSpan.setAttribute(key, value); } + @Override + public void setError(Exception exception) { + delegateSpan.setStatus(StatusCode.ERROR, exception.getMessage()); + } + @Override public void addEvent(String event) { delegateSpan.addEvent(event); diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java index ce5e0989b622f..2b879f1c37d88 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationIT.java @@ -435,6 +435,7 @@ public void testNodeDropWithOngoingReplication() throws Exception { refresh(INDEX_NAME); blockFileCopy.countDown(); internalCluster().stopRandomNode(InternalTestCluster.nameFilter(primaryNode)); + ensureYellow(INDEX_NAME); assertBusy(() -> { assertDocCounts(docCount, replicaNode); }); state = client().admin().cluster().prepareState().execute().actionGet().getState(); // replica now promoted as primary should have same allocation id @@ -799,6 +800,7 @@ public void testReplicaHasDiffFilesThanPrimary() throws Exception { public void testPressureServiceStats() throws Exception { final String primaryNode = internalCluster().startDataOnlyNode(); createIndex(INDEX_NAME); + ensureYellow(INDEX_NAME); final String replicaNode = internalCluster().startDataOnlyNode(); ensureGreen(INDEX_NAME); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java index 77de601b53ec6..be8976671d04f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreIT.java @@ -320,6 +320,6 @@ public void testStaleCommitDeletionWithoutInvokeFlush() throws Exception { .get() .getSetting(INDEX_NAME, IndexMetadata.SETTING_INDEX_UUID); Path indexPath = Path.of(String.valueOf(absolutePath), indexUUID, "/0/segments/metadata"); - assertEquals(1, getFileCount(indexPath)); + assertEquals(numberOfIterations, getFileCount(indexPath)); } } diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java index 01fb91f83aa02..d39b30ada5ef7 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java @@ -64,7 +64,6 @@ public void teardown() { assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME)); } - @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/7592") @Override public void testPressureServiceStats() throws Exception { super.testPressureServiceStats(); diff --git a/server/src/main/java/org/opensearch/action/ActionModule.java b/server/src/main/java/org/opensearch/action/ActionModule.java index 902ae7cc54e3f..56d1758161ced 100644 --- a/server/src/main/java/org/opensearch/action/ActionModule.java +++ b/server/src/main/java/org/opensearch/action/ActionModule.java @@ -462,9 +462,9 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; @@ -1044,7 +1044,7 @@ public static class DynamicActionRegistry { // A dynamic registry to add or remove Route / RestSendToExtensionAction pairs // at times other than node bootstrap. - private final Map routeRegistry = new ConcurrentHashMap<>(); + private final Map routeRegistry = new ConcurrentHashMap<>(); private final Set registeredActionNames = new ConcurrentSkipListSet<>(); @@ -1112,26 +1112,37 @@ public boolean isActionRegistered(String actionName) { } /** - * Add a dynamic action to the registry. + * Adds a dynamic route to the registry. * * @param route The route instance to add * @param action The corresponding instance of RestSendToExtensionAction to execute */ - public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAction action) { + public void registerDynamicRoute(NamedRoute route, RestSendToExtensionAction action) { requireNonNull(route, "route is required"); requireNonNull(action, "action is required"); - Optional routeName = Optional.empty(); - if (route instanceof NamedRoute) { - routeName = Optional.of(((NamedRoute) route).name()); - if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) { - throw new IllegalArgumentException("route [" + route + "] already registered"); - } + + String routeName = route.name(); + requireNonNull(routeName, "route name is required"); + if (isActionRegistered(routeName)) { + throw new IllegalArgumentException("route [" + route + "] already registered"); } + + Set actionNames = route.actionNames(); + if (!Collections.disjoint(actionNames, registeredActionNames)) { + Set alreadyRegistered = new HashSet<>(registeredActionNames); + alreadyRegistered.retainAll(actionNames); + String acts = String.join(", ", alreadyRegistered); + throw new IllegalArgumentException( + "action" + (alreadyRegistered.size() > 1 ? "s [" : " [") + acts + "] already registered" + ); + } + if (routeRegistry.containsKey(route)) { throw new IllegalArgumentException("route [" + route + "] already registered"); } routeRegistry.put(route, action); - routeName.ifPresent(registeredActionNames::add); + registeredActionNames.add(routeName); + registeredActionNames.addAll(actionNames); } /** @@ -1139,14 +1150,14 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct * * @param route The route to remove */ - public void unregisterDynamicRoute(RestHandler.Route route) { + public void unregisterDynamicRoute(NamedRoute route) { requireNonNull(route, "route is required"); if (routeRegistry.remove(route) == null) { throw new IllegalArgumentException("action [" + route + "] was not registered"); } - if (route instanceof NamedRoute) { - registeredActionNames.remove(((NamedRoute) route).name()); - } + + registeredActionNames.remove(route.name()); + registeredActionNames.removeAll(route.actionNames()); } /** 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 6b8e06594acb7..4cdc54f9c7952 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 @@ -193,7 +193,7 @@ public NodeStats(StreamInput in) throws IOException { } else { taskCancellationStats = null; } - if (in.getVersion().onOrAfter(Version.V_3_0_0)) { // TODO Update to 2_9_0 when we backport to 2.x + if (in.getVersion().onOrAfter(Version.V_2_9_0)) { searchPipelineStats = in.readOptionalWriteable(SearchPipelineStats::new); } else { searchPipelineStats = null; @@ -427,7 +427,7 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_2_9_0)) { out.writeOptionalWriteable(taskCancellationStats); } - if (out.getVersion().onOrAfter(Version.V_3_0_0)) { // TODO: Update to 2_9_0 once we backport to 2.x + if (out.getVersion().onOrAfter(Version.V_2_9_0)) { out.writeOptionalWriteable(searchPipelineStats); } } diff --git a/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java b/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java index f47f342617732..878673b77a4a9 100644 --- a/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java +++ b/server/src/main/java/org/opensearch/extensions/rest/RestInitializeExtensionAction.java @@ -22,6 +22,7 @@ import org.opensearch.extensions.ExtensionsSettings.Extension; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.NamedRoute; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestStatus; import org.opensearch.transport.ConnectTransportException; @@ -54,7 +55,7 @@ public String getName() { @Override public List routes() { - return List.of(new Route(POST, "/_extensions/initialize")); + return List.of(new NamedRoute.Builder().method(POST).path("/_extensions/initialize").uniqueName("extensions:initialize").build()); } public RestInitializeExtensionAction(ExtensionsManager extensionsManager) { @@ -187,6 +188,5 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client channel.sendResponse(new BytesRestResponse(RestStatus.ACCEPTED, builder)); } }; - } } diff --git a/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java b/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java index 073b3f3f45818..3dd6056bb36cf 100644 --- a/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java +++ b/server/src/main/java/org/opensearch/extensions/rest/RestSendToExtensionAction.java @@ -33,9 +33,9 @@ import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.concurrent.CompletableFuture; @@ -90,33 +90,43 @@ public RestSendToExtensionAction( List restActionsAsRoutes = new ArrayList<>(); for (String restAction : restActionsRequest.getRestActions()) { - Optional name = Optional.empty(); + + // TODO Find a better way to parse these to avoid code-smells + + String name; + Set actionNames = new HashSet<>(); String[] parts = restAction.split(" "); - if (parts.length < 2) { - throw new IllegalArgumentException("REST action must contain at least a REST method and route"); + if (parts.length < 3) { + throw new IllegalArgumentException("REST action must contain at least a REST method, a route and a unique name"); } try { method = RestRequest.Method.valueOf(parts[0].trim()); path = pathPrefix + parts[1].trim(); - if (parts.length > 2) { - name = Optional.of(parts[2].trim()); + name = parts[2].trim(); + + // comma-separated action names + if (parts.length > 3) { + String[] actions = parts[3].split(","); + for (String action : actions) { + String trimmed = action.trim(); + if (!trimmed.isEmpty()) { + actionNames.add(trimmed); + } + } } } catch (IndexOutOfBoundsException | IllegalArgumentException e) { throw new IllegalArgumentException(restAction + " does not begin with a valid REST method"); } - logger.info("Registering: " + method + " " + path); - if (name.isPresent()) { - NamedRoute nr = new NamedRoute(method, path, name.get()); - restActionsAsRoutes.add(nr); - dynamicActionRegistry.registerDynamicRoute(nr, this); - } else { - Route r = new Route(method, path); - restActionsAsRoutes.add(r); - dynamicActionRegistry.registerDynamicRoute(r, this); - } + logger.info("Registering: " + method + " " + path + " " + name); + + // All extension routes being registered must have a unique name associated with them + NamedRoute nr = new NamedRoute.Builder().method(method).path(path).uniqueName(name).legacyActionNames(actionNames).build(); + restActionsAsRoutes.add(nr); + dynamicActionRegistry.registerDynamicRoute(nr, this); } this.routes = unmodifiableList(restActionsAsRoutes); + // TODO: Modify {@link NamedRoute} to support deprecated route registration List restActionsAsDeprecatedRoutes = new ArrayList<>(); // Iterate in pairs of route / deprecation message List deprecatedActions = restActionsRequest.getDeprecatedRestActions(); diff --git a/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java b/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java deleted file mode 100644 index 189d67c120189..0000000000000 --- a/server/src/main/java/org/opensearch/extensions/rest/RouteHandler.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * 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.extensions.rest; - -import java.util.function.Function; - -import org.opensearch.rest.RestHandler.Route; -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestRequest.Method; - -/** - * A subclass of {@link Route} that includes a handler method for that route. - */ -public class RouteHandler extends Route { - - private final String name; - - private final Function responseHandler; - - /** - * Handle the method and path with the specified handler. - * - * @param method The {@link Method} to handle. - * @param path The path to handle. - * @param handler The method which handles the method and path. - */ - public RouteHandler(Method method, String path, Function handler) { - super(method, path); - this.responseHandler = handler; - this.name = null; - } - - /** - * Handle the method and path with the specified handler. - * - * @param name The name of the handler. - * @param method The {@link Method} to handle. - * @param path The path to handle. - * @param handler The method which handles the method and path. - */ - public RouteHandler(String name, Method method, String path, Function handler) { - super(method, path); - this.responseHandler = handler; - this.name = name; - } - - /** - * Executes the handler for this route. - * - * @param request The request to handle - * @return the {@link ExtensionRestResponse} result from the handler for this route. - */ - public ExtensionRestResponse handleRequest(RestRequest request) { - return responseHandler.apply(request); - } - - /** - * The name of the RouteHandler. Must be unique across route handlers. - */ - public String name() { - return this.name; - } -} diff --git a/server/src/main/java/org/opensearch/index/codec/customcodecs/Lucene95CustomCodec.java b/server/src/main/java/org/opensearch/index/codec/customcodecs/Lucene95CustomCodec.java index 3c570f9d0566c..8aa422a47a073 100644 --- a/server/src/main/java/org/opensearch/index/codec/customcodecs/Lucene95CustomCodec.java +++ b/server/src/main/java/org/opensearch/index/codec/customcodecs/Lucene95CustomCodec.java @@ -23,7 +23,7 @@ * @opensearch.internal */ public abstract class Lucene95CustomCodec extends FilterCodec { - public static final int DEFAULT_COMPRESSION_LEVEL = 6; + public static final int DEFAULT_COMPRESSION_LEVEL = 3; /** Each mode represents a compression algorithm. */ public enum Mode { diff --git a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java index ddca12d9283f3..cd3e7aa3b11a9 100644 --- a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java +++ b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java @@ -30,6 +30,7 @@ import org.opensearch.index.seqno.SequenceNumbers; import org.opensearch.index.store.RemoteSegmentStoreDirectory; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; +import org.opensearch.index.translog.Translog; import org.opensearch.indices.replication.checkpoint.ReplicationCheckpoint; import org.opensearch.indices.replication.checkpoint.SegmentReplicationCheckpointPublisher; import org.opensearch.threadpool.Scheduler; @@ -224,7 +225,17 @@ private synchronized void syncSegments(boolean isRetry) { // Each metadata file in the remote segment store represents a commit and the following // statement keeps sure that each metadata will always contain all the segments from last commit + refreshed // segments. - localSegmentsPostRefresh.addAll(SegmentInfos.readCommit(storeDirectory, latestSegmentInfos.get()).files(true)); + SegmentInfos segmentCommitInfos; + try { + segmentCommitInfos = SegmentInfos.readCommit(storeDirectory, latestSegmentInfos.get()); + } catch (Exception e) { + // Seeing discrepancy in segment infos and files on disk. SegmentInfosSnapshot is returning + // a segment_N file which does not exist on local disk. + logger.error("Exception occurred while SegmentInfos.readCommit(..)", e); + logger.error("segmentInfosFiles={} diskFiles={}", localSegmentsPostRefresh, storeDirectory.listAll()); + throw e; + } + localSegmentsPostRefresh.addAll(segmentCommitInfos.files(true)); segmentInfosFiles.stream() .filter(file -> !file.equals(latestSegmentInfos.get())) .forEach(localSegmentsPostRefresh::remove); @@ -349,12 +360,19 @@ void uploadMetadata(Collection localSegmentsPostRefresh, SegmentInfos se userData.put(SequenceNumbers.MAX_SEQ_NO, Long.toString(maxSeqNo)); segmentInfosSnapshot.setUserData(userData, false); - remoteDirectory.uploadMetadata( - localSegmentsPostRefresh, - segmentInfosSnapshot, - storeDirectory, - indexShard.getOperationPrimaryTerm() - ); + Translog.TranslogGeneration translogGeneration = indexShard.getEngine().translogManager().getTranslogGeneration(); + if (translogGeneration == null) { + throw new UnsupportedOperationException("Encountered null TranslogGeneration while uploading metadata to remote segment store"); + } else { + long translogFileGeneration = translogGeneration.translogFileGeneration; + remoteDirectory.uploadMetadata( + localSegmentsPostRefresh, + segmentInfosSnapshot, + storeDirectory, + indexShard.getOperationPrimaryTerm(), + translogFileGeneration + ); + } } private boolean uploadNewSegments(Collection localSegmentsPostRefresh) throws IOException { diff --git a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java index be4b4e910bb4d..8782808c070ab 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java @@ -13,6 +13,8 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; +import org.opensearch.action.ActionListener; +import org.opensearch.action.LatchedActionListener; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.blobstore.BlobMetadata; @@ -20,10 +22,15 @@ import java.io.IOException; import java.io.InputStream; import java.nio.file.NoSuchFileException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; /** * A {@code RemoteDirectory} provides an abstraction layer for storing a list of files to a remote store. @@ -61,6 +68,40 @@ public Collection listFilesByPrefix(String filenamePrefix) throws IOExce return blobContainer.listBlobsByPrefix(filenamePrefix).keySet(); } + public List listFilesByPrefixInLexicographicOrder(String filenamePrefix, int limit) throws IOException { + List sortedBlobList = new ArrayList<>(); + AtomicReference exception = new AtomicReference<>(); + final CountDownLatch latch = new CountDownLatch(1); + LatchedActionListener> actionListener = new LatchedActionListener<>(new ActionListener<>() { + @Override + public void onResponse(List blobMetadata) { + sortedBlobList.addAll(blobMetadata.stream().map(BlobMetadata::name).collect(Collectors.toList())); + } + + @Override + public void onFailure(Exception e) { + exception.set(e); + } + }, latch); + + try { + blobContainer.listBlobsByPrefixInSortedOrder( + filenamePrefix, + limit, + BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC, + actionListener + ); + latch.await(); + } catch (InterruptedException e) { + throw new IOException("Exception in listFilesByPrefixInLexicographicOrder with prefix: " + filenamePrefix, e); + } + if (exception.get() != null) { + throw new IOException(exception.get()); + } else { + return sortedBlobList; + } + } + /** * Removes an existing file in the directory. * diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java index ac129aca8baf7..e7602203440d2 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java @@ -22,6 +22,7 @@ import org.opensearch.common.UUIDs; import org.opensearch.common.io.VersionedCodecStreamWrapper; import org.opensearch.common.lucene.store.ByteArrayIndexInput; +import org.opensearch.index.remote.RemoteStoreUtils; import org.opensearch.index.store.lockmanager.FileLockInfo; import org.opensearch.index.store.lockmanager.RemoteStoreCommitLevelLockManager; import org.opensearch.index.store.lockmanager.RemoteStoreLockManager; @@ -34,15 +35,14 @@ import java.nio.file.NoSuchFileException; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; /** @@ -62,9 +62,6 @@ public final class RemoteSegmentStoreDirectory extends FilterDirectory implement */ public static final String SEGMENT_NAME_UUID_SEPARATOR = "__"; - public static final MetadataFilenameUtils.MetadataFilenameComparator METADATA_FILENAME_COMPARATOR = - new MetadataFilenameUtils.MetadataFilenameComparator(); - /** * remoteDataDirectory is used to store segment files at path: cluster_UUID/index_UUID/shardId/segments/data */ @@ -78,12 +75,6 @@ public final class RemoteSegmentStoreDirectory extends FilterDirectory implement private final ThreadPool threadPool; - /** - * To prevent explosion of refresh metadata files, we replace refresh files for the given primary term and generation - * This is achieved by uploading refresh metadata file with the same UUID suffix. - */ - private String commonFilenameSuffix; - /** * Keeps track of local segment filename to uploaded filename along with other attributes like checksum. * This map acts as a cache layer for uploaded segment filenames which helps avoid calling listAll() each time. @@ -105,6 +96,8 @@ public final class RemoteSegmentStoreDirectory extends FilterDirectory implement */ protected final AtomicBoolean canDeleteStaleCommits = new AtomicBoolean(true); + private final AtomicLong metadataUploadCounter = new AtomicLong(0); + public RemoteSegmentStoreDirectory( RemoteDirectory remoteDataDirectory, RemoteDirectory remoteMetadataDirectory, @@ -127,7 +120,6 @@ public RemoteSegmentStoreDirectory( * @throws IOException if there were any failures in reading the metadata file */ public RemoteSegmentMetadata init() throws IOException { - this.commonFilenameSuffix = UUIDs.base64UUID(); RemoteSegmentMetadata remoteSegmentMetadata = readLatestMetadataFile(); if (remoteSegmentMetadata != null) { this.segmentsUploadedToRemoteStore = new ConcurrentHashMap<>(remoteSegmentMetadata.getMetadata()); @@ -170,12 +162,15 @@ public RemoteSegmentMetadata initializeToSpecificCommit(long primaryTerm, long c public RemoteSegmentMetadata readLatestMetadataFile() throws IOException { RemoteSegmentMetadata remoteSegmentMetadata = null; - Collection metadataFiles = remoteMetadataDirectory.listFilesByPrefix(MetadataFilenameUtils.METADATA_PREFIX); - Optional latestMetadataFile = metadataFiles.stream().max(METADATA_FILENAME_COMPARATOR); + List metadataFiles = remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + MetadataFilenameUtils.METADATA_PREFIX, + 1 + ); - if (latestMetadataFile.isPresent()) { - logger.info("Reading latest Metadata file {}", latestMetadataFile.get()); - remoteSegmentMetadata = readMetadataFile(latestMetadataFile.get()); + if (metadataFiles.isEmpty() == false) { + String latestMetadataFile = metadataFiles.get(0); + logger.info("Reading latest Metadata file {}", latestMetadataFile); + remoteSegmentMetadata = readMetadataFile(latestMetadataFile); } else { logger.info("No metadata file found, this can happen for new index with no data uploaded to remote segment store"); } @@ -187,8 +182,7 @@ private RemoteSegmentMetadata readMetadataFile(String metadataFilename) throws I try (IndexInput indexInput = remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)) { byte[] metadataBytes = new byte[(int) indexInput.length()]; indexInput.readBytes(metadataBytes, 0, (int) indexInput.length()); - RemoteSegmentMetadata metadata = metadataStreamWrapper.readStream(new ByteArrayIndexInput(metadataFilename, metadataBytes)); - return metadata; + return metadataStreamWrapper.readStream(new ByteArrayIndexInput(metadataFilename, metadataBytes)); } } @@ -242,56 +236,43 @@ static class MetadataFilenameUtils { public static final String SEPARATOR = "__"; public static final String METADATA_PREFIX = "metadata"; - /** - * Comparator to sort the metadata filenames. The order of sorting is: Primary Term, Generation, UUID - * Even though UUID sort does not provide any info on recency, it provides a consistent way to sort the filenames. - */ - static class MetadataFilenameComparator implements Comparator { - @Override - public int compare(String first, String second) { - String[] firstTokens = first.split(SEPARATOR); - String[] secondTokens = second.split(SEPARATOR); - if (!firstTokens[0].equals(secondTokens[0])) { - return firstTokens[0].compareTo(secondTokens[0]); - } - long firstPrimaryTerm = getPrimaryTerm(firstTokens); - long secondPrimaryTerm = getPrimaryTerm(secondTokens); - if (firstPrimaryTerm != secondPrimaryTerm) { - return firstPrimaryTerm > secondPrimaryTerm ? 1 : -1; - } else { - long firstGeneration = getGeneration(firstTokens); - long secondGeneration = getGeneration(secondTokens); - if (firstGeneration != secondGeneration) { - return firstGeneration > secondGeneration ? 1 : -1; - } else { - return getUuid(firstTokens).compareTo(getUuid(secondTokens)); - } - } - } - } - static String getMetadataFilePrefixForCommit(long primaryTerm, long generation) { - return String.join(SEPARATOR, METADATA_PREFIX, Long.toString(primaryTerm), Long.toString(generation, Character.MAX_RADIX)); + return String.join( + SEPARATOR, + METADATA_PREFIX, + RemoteStoreUtils.invertLong(primaryTerm), + RemoteStoreUtils.invertLong(generation) + ); } // Visible for testing - static String getMetadataFilename(long primaryTerm, long generation, String uuid) { - return String.join(SEPARATOR, getMetadataFilePrefixForCommit(primaryTerm, generation), uuid); + static String getMetadataFilename( + long primaryTerm, + long generation, + long translogGeneration, + long uploadCounter, + int metadataVersion + ) { + return String.join( + SEPARATOR, + METADATA_PREFIX, + RemoteStoreUtils.invertLong(primaryTerm), + RemoteStoreUtils.invertLong(generation), + RemoteStoreUtils.invertLong(translogGeneration), + RemoteStoreUtils.invertLong(uploadCounter), + RemoteStoreUtils.invertLong(System.currentTimeMillis()), + String.valueOf(metadataVersion) + ); } // Visible for testing static long getPrimaryTerm(String[] filenameTokens) { - return Long.parseLong(filenameTokens[1]); + return RemoteStoreUtils.invertLong(filenameTokens[1]); } // Visible for testing static long getGeneration(String[] filenameTokens) { - return Long.parseLong(filenameTokens[2], Character.MAX_RADIX); - } - - // Visible for testing - static String getUuid(String[] filenameTokens) { - return filenameTokens[3]; + return RemoteStoreUtils.invertLong(filenameTokens[2]); } } @@ -379,7 +360,6 @@ public IndexInput openInput(String name, IOContext context) throws IOException { @Override public void acquireLock(long primaryTerm, long generation, String acquirerId) throws IOException { String metadataFile = getMetadataFileForCommit(primaryTerm, generation); - mdLockManager.acquire(FileLockInfo.getLockInfoBuilder().withFileToLock(metadataFile).withAcquirerId(acquirerId).build()); } @@ -408,13 +388,19 @@ public void releaseLock(long primaryTerm, long generation, String acquirerId) th @Override public Boolean isLockAcquired(long primaryTerm, long generation) throws IOException { String metadataFile = getMetadataFileForCommit(primaryTerm, generation); + return isLockAcquired(metadataFile); + } + + // Visible for testing + Boolean isLockAcquired(String metadataFile) throws IOException { return mdLockManager.isAcquired(FileLockInfo.getLockInfoBuilder().withFileToLock(metadataFile).build()); } // Visible for testing String getMetadataFileForCommit(long primaryTerm, long generation) throws IOException { - Collection metadataFiles = remoteMetadataDirectory.listFilesByPrefix( - MetadataFilenameUtils.getMetadataFilePrefixForCommit(primaryTerm, generation) + List metadataFiles = remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + MetadataFilenameUtils.getMetadataFilePrefixForCommit(primaryTerm, generation), + 1 ); if (metadataFiles.isEmpty()) { @@ -432,33 +418,24 @@ String getMetadataFileForCommit(long primaryTerm, long generation) throws IOExce + metadataFiles.size() ); } - return metadataFiles.iterator().next(); + return metadataFiles.get(0); } - public void copyFrom(Directory from, String src, String dest, IOContext context, boolean useCommonSuffix, String checksum) - throws IOException { + public void copyFrom(Directory from, String src, String dest, IOContext context, String checksum) throws IOException { String remoteFilename; - if (useCommonSuffix) { - remoteFilename = dest + SEGMENT_NAME_UUID_SEPARATOR + this.commonFilenameSuffix; - } else { - remoteFilename = getNewRemoteSegmentFilename(dest); - } + remoteFilename = getNewRemoteSegmentFilename(dest); remoteDataDirectory.copyFrom(from, src, remoteFilename, context); UploadedSegmentMetadata segmentMetadata = new UploadedSegmentMetadata(src, remoteFilename, checksum, from.fileLength(src)); segmentsUploadedToRemoteStore.put(src, segmentMetadata); } - public void copyFrom(Directory from, String src, String dest, IOContext context, boolean useCommonSuffix) throws IOException { - copyFrom(from, src, dest, context, useCommonSuffix, getChecksumOfLocalFile(from, src)); - } - /** * Copies an existing src file from directory from to a non-existent file dest in this directory. * Once the segment is uploaded to remote segment store, update the cache accordingly. */ @Override public void copyFrom(Directory from, String src, String dest, IOContext context) throws IOException { - copyFrom(from, src, dest, context, false); + copyFrom(from, src, dest, context, getChecksumOfLocalFile(from, src)); } /** @@ -486,13 +463,16 @@ public void uploadMetadata( Collection segmentFiles, SegmentInfos segmentInfosSnapshot, Directory storeDirectory, - long primaryTerm + long primaryTerm, + long translogGeneration ) throws IOException { synchronized (this) { String metadataFilename = MetadataFilenameUtils.getMetadataFilename( primaryTerm, segmentInfosSnapshot.getGeneration(), - this.commonFilenameSuffix + translogGeneration, + metadataUploadCounter.incrementAndGet(), + RemoteSegmentMetadata.CURRENT_VERSION ); try { IndexOutput indexOutput = storeDirectory.createOutput(metadataFilename, IOContext.DEFAULT); @@ -569,15 +549,6 @@ public Map getSegmentsUploadedToRemoteStore() { return Collections.unmodifiableMap(this.segmentsUploadedToRemoteStore); } - public Map getSegmentsUploadedToRemoteStore(long primaryTerm, long generation) throws IOException { - String metadataFile = getMetadataFileForCommit(primaryTerm, generation); - - Map segmentsUploadedToRemoteStore = new ConcurrentHashMap<>( - readMetadataFile(metadataFile).getMetadata() - ); - return Collections.unmodifiableMap(segmentsUploadedToRemoteStore); - } - /** * Delete stale segment and metadata files * One metadata file is kept per commit (refresh updates the same file). To read segments uploaded to remote store, @@ -585,9 +556,11 @@ public Map getSegmentsUploadedToRemoteStore(lon * @param lastNMetadataFilesToKeep number of metadata files to keep * @throws IOException in case of I/O error while reading from / writing to remote segment store */ - private void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOException { - Collection metadataFiles = remoteMetadataDirectory.listFilesByPrefix(MetadataFilenameUtils.METADATA_PREFIX); - List sortedMetadataFileList = metadataFiles.stream().sorted(METADATA_FILENAME_COMPARATOR).collect(Collectors.toList()); + public void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOException { + List sortedMetadataFileList = remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + MetadataFilenameUtils.METADATA_PREFIX, + Integer.MAX_VALUE + ); if (sortedMetadataFileList.size() <= lastNMetadataFilesToKeep) { logger.info( "Number of commits in remote segment store={}, lastNMetadataFilesToKeep={}", @@ -598,21 +571,12 @@ private void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOExceptio } List metadataFilesEligibleToDelete = sortedMetadataFileList.subList( - 0, - sortedMetadataFileList.size() - lastNMetadataFilesToKeep + lastNMetadataFilesToKeep, + sortedMetadataFileList.size() ); List metadataFilesToBeDeleted = metadataFilesEligibleToDelete.stream().filter(metadataFile -> { try { - // TODO: add snapshot interop feature flag here as that will be the first feature to use lock - // manager. - boolean lockManagerEnabled = false; - if (!lockManagerEnabled) { - return true; - } - return !isLockAcquired( - MetadataFilenameUtils.getPrimaryTerm(metadataFile.split(MetadataFilenameUtils.SEPARATOR)), - MetadataFilenameUtils.getGeneration(metadataFile.split(MetadataFilenameUtils.SEPARATOR)) - ); + return !isLockAcquired(metadataFile); } catch (IOException e) { logger.error( "skipping metadata file (" @@ -699,7 +663,10 @@ public void deleteStaleSegmentsAsync(int lastNMetadataFilesToKeep) { Return true if it deleted it successfully */ private boolean deleteIfEmpty() throws IOException { - Collection metadataFiles = remoteMetadataDirectory.listFilesByPrefix(MetadataFilenameUtils.METADATA_PREFIX); + Collection metadataFiles = remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + MetadataFilenameUtils.METADATA_PREFIX, + 1 + ); if (metadataFiles.size() != 0) { logger.info("Remote directory still has files , not deleting the path"); return false; diff --git a/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java b/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java index 7eaab67ddb5a5..9b3240823f368 100644 --- a/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java +++ b/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java @@ -300,6 +300,11 @@ public void onDelete() { translog.onDelete(); } + @Override + public Translog.TranslogGeneration getTranslogGeneration() { + return translog.getGeneration(); + } + /** * Reads operations from the translog * @param location location of translog diff --git a/server/src/main/java/org/opensearch/index/translog/NoOpTranslogManager.java b/server/src/main/java/org/opensearch/index/translog/NoOpTranslogManager.java index 58ee8c0fd39e7..dd5593b6d79cd 100644 --- a/server/src/main/java/org/opensearch/index/translog/NoOpTranslogManager.java +++ b/server/src/main/java/org/opensearch/index/translog/NoOpTranslogManager.java @@ -122,4 +122,9 @@ public Translog.Snapshot newChangesSnapshot(long fromSeqNo, long toSeqNo, boolea } public void onDelete() {} + + @Override + public Translog.TranslogGeneration getTranslogGeneration() { + return null; + } } diff --git a/server/src/main/java/org/opensearch/index/translog/TranslogManager.java b/server/src/main/java/org/opensearch/index/translog/TranslogManager.java index 420d6cdc43bbf..78aaa1bc13a00 100644 --- a/server/src/main/java/org/opensearch/index/translog/TranslogManager.java +++ b/server/src/main/java/org/opensearch/index/translog/TranslogManager.java @@ -131,4 +131,6 @@ public interface TranslogManager { Clean up if any needed on deletion of index */ void onDelete(); + + Translog.TranslogGeneration getTranslogGeneration(); } diff --git a/server/src/main/java/org/opensearch/rest/NamedRoute.java b/server/src/main/java/org/opensearch/rest/NamedRoute.java index f5eaafcd04056..109f688a4924e 100644 --- a/server/src/main/java/org/opensearch/rest/NamedRoute.java +++ b/server/src/main/java/org/opensearch/rest/NamedRoute.java @@ -9,6 +9,13 @@ package org.opensearch.rest; import org.opensearch.OpenSearchException; +import org.opensearch.transport.TransportService; + +import java.util.HashSet; +import java.util.Set; +import java.util.function.Function; + +import static java.util.Objects.requireNonNull; /** * A named Route @@ -16,21 +23,123 @@ * @opensearch.internal */ public class NamedRoute extends RestHandler.Route { + private static final String VALID_ACTION_NAME_PATTERN = "^[a-zA-Z0-9:/*_]*$"; static final int MAX_LENGTH_OF_ACTION_NAME = 250; - private final String name; + private final String uniqueName; + private final Set actionNames; - public boolean isValidRouteName(String routeName) { - if (routeName == null || routeName.isBlank() || routeName.length() > MAX_LENGTH_OF_ACTION_NAME) { - return false; + private Function handler; + + /** + * Builder class for constructing instances of {@link NamedRoute}. + */ + public static class Builder { + private RestRequest.Method method; + private String path; + private String uniqueName; + private final Set legacyActionNames = new HashSet<>(); + private Function handler; + + /** + * Sets the REST method for the route. + * + * @param method the REST method for the route + * @return the builder instance + */ + public Builder method(RestRequest.Method method) { + requireNonNull(method, "REST method must not be null."); + this.method = method; + return this; + } + + /** + * Sets the URL path for the route. + * + * @param path the URL path for the route + * @return the builder instance + */ + public Builder path(String path) { + requireNonNull(path, "REST path must not be null."); + this.path = path; + return this; + } + + /** + * Sets the name for the route. + * + * @param name the name for the route + * @return the builder instance + */ + public Builder uniqueName(String name) { + requireNonNull(name, "REST route name must not be null."); + this.uniqueName = name; + return this; + } + + /** + * Sets the legacy action names for the route. + * + * @param legacyActionNames the legacy action names for the route + * @return the builder instance + */ + public Builder legacyActionNames(Set legacyActionNames) { + this.legacyActionNames.addAll(validateLegacyActionNames(legacyActionNames)); + return this; + } + + /** + * Sets the handler for this route + * + * @param handler the handler for this route + * @return the builder instance + */ + public Builder handler(Function handler) { + requireNonNull(handler, "Route handler must not be null."); + this.handler = handler; + return this; + } + + /** + * Builds a new instance of {@link NamedRoute} based on the provided parameters. + * + * @return a new instance of {@link NamedRoute} + * @throws OpenSearchException if the route name is invalid + */ + public NamedRoute build() { + checkIfFieldsAreSet(); + return new NamedRoute(this); + } + + /** + * Checks if all builder fields are set before creating a new NamedRoute object + */ + private void checkIfFieldsAreSet() { + if (method == null || path == null || uniqueName == null) { + throw new IllegalStateException("REST method, path and uniqueName are required."); + } + } + + private Set validateLegacyActionNames(Set legacyActionNames) { + if (legacyActionNames == null) { + return new HashSet<>(); + } + for (String actionName : legacyActionNames) { + if (!TransportService.isValidActionName(actionName)) { + throw new OpenSearchException( + "Invalid action name [" + actionName + "]. It must start with one of: " + TransportService.VALID_ACTION_PREFIXES + ); + } + } + return legacyActionNames; } - return routeName.matches(VALID_ACTION_NAME_PATTERN); + } - public NamedRoute(RestRequest.Method method, String path, String name) { - super(method, path); - if (!isValidRouteName(name)) { + private NamedRoute(Builder builder) { + super(builder.method, builder.path); + if (!isValidRouteName(builder.uniqueName)) { throw new OpenSearchException( "Invalid route name specified. The route name may include the following characters" + " 'a-z', 'A-Z', '0-9', ':', '/', '*', '_' and be less than " @@ -38,18 +147,43 @@ public NamedRoute(RestRequest.Method method, String path, String name) { + " characters" ); } - this.name = name; + this.uniqueName = builder.uniqueName; + this.actionNames = Set.copyOf(builder.legacyActionNames); + this.handler = builder.handler; + } + + public boolean isValidRouteName(String routeName) { + return routeName != null + && !routeName.isBlank() + && routeName.length() <= MAX_LENGTH_OF_ACTION_NAME + && routeName.matches(VALID_ACTION_NAME_PATTERN); } /** * The name of the Route. Must be unique across Route. */ public String name() { - return this.name; + return this.uniqueName; + } + + /** + * The legacy transport Action name to match against this route to support authorization in REST layer. + * MUST be unique across all Routes + */ + public Set actionNames() { + return this.actionNames; + } + + /** + * The handler associated with this route + * @return the handler associated with this route + */ + public Function handler() { + return handler; } @Override public String toString() { - return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + "]"; + return "NamedRoute [method=" + method + ", path=" + path + ", name=" + uniqueName + ", actionNames=" + actionNames + "]"; } } diff --git a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java index 963d47df3baff..17e424ee81e7e 100644 --- a/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java +++ b/server/src/test/java/org/opensearch/action/DynamicActionRegistryTests.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.util.Collections; import java.util.Map; +import java.util.Set; import static org.mockito.Mockito.mock; @@ -80,8 +81,8 @@ public void testDynamicActionRegistry() { public void testDynamicActionRegistryWithNamedRoutes() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); - NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); + NamedRoute r1 = new NamedRoute.Builder().method(RestRequest.Method.GET).path("/foo").uniqueName("foo").build(); + NamedRoute r2 = new NamedRoute.Builder().method(RestRequest.Method.PUT).path("/bar").uniqueName("bar").build(); DynamicActionRegistry registry = new DynamicActionRegistry(); registry.registerDynamicRoute(r1, action); @@ -89,22 +90,38 @@ public void testDynamicActionRegistryWithNamedRoutes() { assertTrue(registry.isActionRegistered("foo")); assertTrue(registry.isActionRegistered("bar")); + + registry.unregisterDynamicRoute(r2); + + assertTrue(registry.isActionRegistered("foo")); + assertFalse(registry.isActionRegistered("bar")); } - public void testDynamicActionRegistryRegisterAndUnregisterWithNamedRoutes() { + public void testDynamicActionRegistryWithNamedRoutesAndLegacyActionNames() { RestSendToExtensionAction action = mock(RestSendToExtensionAction.class); RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class); - NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo"); - NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar"); + NamedRoute r1 = new NamedRoute.Builder().method(RestRequest.Method.GET) + .path("/foo") + .uniqueName("foo") + .legacyActionNames(Set.of("cluster:admin/opensearch/abc/foo")) + .build(); + NamedRoute r2 = new NamedRoute.Builder().method(RestRequest.Method.PUT) + .path("/bar") + .uniqueName("bar") + .legacyActionNames(Set.of("cluster:admin/opensearch/xyz/bar")) + .build(); DynamicActionRegistry registry = new DynamicActionRegistry(); registry.registerDynamicRoute(r1, action); registry.registerDynamicRoute(r2, action2); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo")); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar")); + registry.unregisterDynamicRoute(r2); - assertTrue(registry.isActionRegistered("foo")); - assertFalse(registry.isActionRegistered("bar")); + assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo")); + assertFalse(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar")); } private static final class TestAction extends ActionType { diff --git a/server/src/test/java/org/opensearch/common/blobstore/fs/FsBlobContainerTests.java b/server/src/test/java/org/opensearch/common/blobstore/fs/FsBlobContainerTests.java index f139a5d4e3bb1..4a2eeabeb7e58 100644 --- a/server/src/test/java/org/opensearch/common/blobstore/fs/FsBlobContainerTests.java +++ b/server/src/test/java/org/opensearch/common/blobstore/fs/FsBlobContainerTests.java @@ -131,7 +131,7 @@ private void testListBlobsByPrefixInSortedOrder(int limit, BlobContainer.BlobNam List blobsInFileSystem = new ArrayList<>(); for (int i = 0; i < 10; i++) { - final String blobName = randomAlphaOfLengthBetween(1, 20).toLowerCase(Locale.ROOT); + final String blobName = randomAlphaOfLengthBetween(10, 20).toLowerCase(Locale.ROOT); final byte[] blobData = randomByteArrayOfLength(randomIntBetween(1, frequently() ? 512 : 1 << 20)); // rarely up to 1mb Files.write(path.resolve(blobName), blobData); blobsInFileSystem.add(blobName); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 713a70c6a7d3e..3f61d01166fb9 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -443,8 +443,8 @@ public void testHandleRegisterRestActionsRequest() throws Exception { initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; - List actionsList = List.of("GET /foo", "PUT /bar", "POST /baz"); - List deprecatedActionsList = List.of("GET /deprecated/foo", "It's deprecated!"); + List actionsList = List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"); + List deprecatedActionsList = List.of("GET /deprecated/foo foo_deprecated", "It's deprecated!"); RegisterRestActionsRequest registerActionsRequest = new RegisterRestActionsRequest(uniqueIdStr, actionsList, deprecatedActionsList); TransportResponse response = extensionsManager.getRestActionsRequestHandler() .handleRegisterRestActionsRequest(registerActionsRequest, actionModule.getDynamicActionRegistry()); diff --git a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java index fe8792b36f048..23a9169b91e21 100644 --- a/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java +++ b/server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java @@ -135,8 +135,8 @@ public void tearDown() throws Exception { public void testRestSendToExtensionAction() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo", "PUT /bar", "POST /baz"), - List.of("GET /deprecated/foo", "It's deprecated!") + List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"), + List.of("GET /deprecated/foo foo_deprecated", "Its deprecated") ); RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( registerRestActionRequest, @@ -180,9 +180,70 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception { assertEquals("send_to_extension_action", restSendToExtensionAction.getName()); List expected = new ArrayList<>(); String uriPrefix = "/_extensions/_uniqueid1"; - expected.add(new NamedRoute(Method.GET, uriPrefix + "/foo", "foo")); - expected.add(new NamedRoute(Method.PUT, uriPrefix + "/bar", "bar")); - expected.add(new NamedRoute(Method.POST, uriPrefix + "/baz", "baz")); + NamedRoute nr1 = new NamedRoute.Builder().method(Method.GET).path(uriPrefix + "/foo").uniqueName("foo").build(); + + NamedRoute nr2 = new NamedRoute.Builder().method(Method.PUT).path(uriPrefix + "/bar").uniqueName("bar").build(); + + NamedRoute nr3 = new NamedRoute.Builder().method(Method.POST).path(uriPrefix + "/baz").uniqueName("baz").build(); + + expected.add(nr1); + expected.add(nr2); + expected.add(nr3); + + List routes = restSendToExtensionAction.routes(); + assertEquals(expected.size(), routes.size()); + List expectedPaths = expected.stream().map(Route::getPath).collect(Collectors.toList()); + List paths = routes.stream().map(Route::getPath).collect(Collectors.toList()); + List expectedMethods = expected.stream().map(Route::getMethod).collect(Collectors.toList()); + List methods = routes.stream().map(Route::getMethod).collect(Collectors.toList()); + List expectedNames = expected.stream().map(NamedRoute::name).collect(Collectors.toList()); + List names = routes.stream().map(r -> ((NamedRoute) r).name()).collect(Collectors.toList()); + assertTrue(paths.containsAll(expectedPaths)); + assertTrue(expectedPaths.containsAll(paths)); + assertTrue(methods.containsAll(expectedMethods)); + assertTrue(expectedMethods.containsAll(methods)); + assertTrue(expectedNames.containsAll(names)); + } + + public void testRestSendToExtensionActionWithNamedRouteAndLegacyActionName() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of( + "GET /foo foo cluster:admin/opensearch/abc/foo", + "PUT /bar bar cluster:admin/opensearch/jkl/bar,cluster:admin/opendistro/mno/bar*", + "POST /baz baz cluster:admin/opensearch/xyz/baz" + ), + List.of("GET /deprecated/foo foo_deprecated cluster:admin/opensearch/abc/foo_deprecated", "It's deprecated!") + ); + RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( + registerRestActionRequest, + discoveryExtensionNode, + transportService, + dynamicActionRegistry + ); + + assertEquals("send_to_extension_action", restSendToExtensionAction.getName()); + List expected = new ArrayList<>(); + String uriPrefix = "/_extensions/_uniqueid1"; + NamedRoute nr1 = new NamedRoute.Builder().method(Method.GET) + .path(uriPrefix + "/foo") + .uniqueName("foo") + .legacyActionNames(Set.of("cluster:admin/opensearch/abc/foo")) + .build(); + NamedRoute nr2 = new NamedRoute.Builder().method(Method.PUT) + .path(uriPrefix + "/bar") + .uniqueName("bar") + .legacyActionNames(Set.of("cluster:admin/opensearch/jkl/bar", "cluster:admin/opendistro/mno/bar*")) + .build(); + NamedRoute nr3 = new NamedRoute.Builder().method(Method.POST) + .path(uriPrefix + "/baz") + .uniqueName("baz") + .legacyActionNames(Set.of("cluster:admin/opensearch/xyz/baz")) + .build(); + + expected.add(nr1); + expected.add(nr2); + expected.add(nr3); List routes = restSendToExtensionAction.routes(); assertEquals(expected.size(), routes.size()); @@ -192,11 +253,26 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception { List methods = routes.stream().map(Route::getMethod).collect(Collectors.toList()); List expectedNames = expected.stream().map(NamedRoute::name).collect(Collectors.toList()); List names = routes.stream().map(r -> ((NamedRoute) r).name()).collect(Collectors.toList()); + Set expectedActionNames = expected.stream().flatMap(nr -> nr.actionNames().stream()).collect(Collectors.toSet()); + Set actionNames = routes.stream().flatMap(nr -> ((NamedRoute) nr).actionNames().stream()).collect(Collectors.toSet()); assertTrue(paths.containsAll(expectedPaths)); assertTrue(expectedPaths.containsAll(paths)); assertTrue(methods.containsAll(expectedMethods)); assertTrue(expectedMethods.containsAll(methods)); assertTrue(expectedNames.containsAll(names)); + assertTrue(expectedActionNames.containsAll(actionNames)); + } + + public void testRestSendToExtensionActionWithoutUniqueNameShouldFail() { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo", "PUT /bar"), + List.of() + ); + expectThrows( + IllegalArgumentException.class, + () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry) + ); } public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exception { @@ -211,6 +287,18 @@ public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exce ); } + public void testRestSendToExtensionMultipleNamedRoutesWithSameLegacyActionName() throws Exception { + RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( + "uniqueid1", + List.of("GET /foo foo cluster:admin/opensearch/abc/foo", "PUT /bar bar cluster:admin/opensearch/abc/foo"), + List.of() + ); + expectThrows( + IllegalArgumentException.class, + () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry) + ); + } + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", @@ -226,7 +314,7 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDifferentPathParams() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo/{path_param1}", "GET /foo/{path_param2}"), + List.of("GET /foo/{path_param1} fooWithParam", "GET /foo/{path_param2} listFooWithParam"), List.of() ); expectThrows( @@ -235,12 +323,13 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDiffer ); } - public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception { + public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"), + List.of("GET /foo/{path_param} fooWithParam", "GET /foo/{path_param}/list listFooWithParam"), List.of() ); + try { new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry); } catch (IllegalArgumentException e) { @@ -285,8 +374,8 @@ public void testRestSendToExtensionWithNamedRouteCollidingWithNativeTransportAct public void testRestSendToExtensionActionFilterHeaders() throws Exception { RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest( "uniqueid1", - List.of("GET /foo", "PUT /bar", "POST /baz"), - List.of("GET /deprecated/foo", "It's deprecated!") + List.of("GET /foo foo", "PUT /bar bar", "POST /baz baz"), + List.of("GET /deprecated/foo foo_deprecated", "It's deprecated!") ); RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction( registerRestActionRequest, diff --git a/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java b/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java deleted file mode 100644 index 855296b2038f0..0000000000000 --- a/server/src/test/java/org/opensearch/extensions/rest/RouteHandlerTests.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * 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.extensions.rest; - -import org.opensearch.rest.RestRequest; -import org.opensearch.rest.RestStatus; -import org.opensearch.test.OpenSearchTestCase; - -public class RouteHandlerTests extends OpenSearchTestCase { - public void testUnnamedRouteHandler() { - RouteHandler rh = new RouteHandler( - RestRequest.Method.GET, - "/foo/bar", - req -> new ExtensionRestResponse(req, RestStatus.OK, "content") - ); - - assertEquals(null, rh.name()); - } - - public void testNamedRouteHandler() { - RouteHandler rh = new RouteHandler( - "foo", - RestRequest.Method.GET, - "/foo/bar", - req -> new ExtensionRestResponse(req, RestStatus.OK, "content") - ); - - assertEquals("foo", rh.name()); - } -} diff --git a/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java index 15f1585bd1477..8ee5fcf0da9d7 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteDirectoryTests.java @@ -12,6 +12,8 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.junit.Before; +import org.opensearch.action.ActionListener; +import org.opensearch.action.LatchedActionListener; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.blobstore.BlobMetadata; import org.opensearch.common.blobstore.support.PlainBlobMetadata; @@ -23,15 +25,19 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.mockito.Mockito.mock; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.doAnswer; public class RemoteDirectoryTests extends OpenSearchTestCase { private BlobContainer blobContainer; @@ -146,6 +152,54 @@ public void testFileLengthIOException() throws IOException { assertThrows(IOException.class, () -> remoteDirectory.fileLength("segment_1")); } + public void testListFilesByPrefixInLexicographicOrder() throws IOException { + doAnswer(invocation -> { + LatchedActionListener> latchedActionListener = invocation.getArgument(3); + latchedActionListener.onResponse(List.of(new PlainBlobMetadata("metadata_1", 1))); + return null; + }).when(blobContainer) + .listBlobsByPrefixInSortedOrder( + eq("metadata"), + eq(1), + eq(BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC), + any(ActionListener.class) + ); + + assertEquals(List.of("metadata_1"), remoteDirectory.listFilesByPrefixInLexicographicOrder("metadata", 1)); + } + + public void testListFilesByPrefixInLexicographicOrderEmpty() throws IOException { + doAnswer(invocation -> { + LatchedActionListener> latchedActionListener = invocation.getArgument(3); + latchedActionListener.onResponse(List.of()); + return null; + }).when(blobContainer) + .listBlobsByPrefixInSortedOrder( + eq("metadata"), + eq(1), + eq(BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC), + any(ActionListener.class) + ); + + assertEquals(List.of(), remoteDirectory.listFilesByPrefixInLexicographicOrder("metadata", 1)); + } + + public void testListFilesByPrefixInLexicographicOrderException() { + doAnswer(invocation -> { + LatchedActionListener> latchedActionListener = invocation.getArgument(3); + latchedActionListener.onFailure(new IOException("Error")); + return null; + }).when(blobContainer) + .listBlobsByPrefixInSortedOrder( + eq("metadata"), + eq(1), + eq(BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC), + any(ActionListener.class) + ); + + assertThrows(IOException.class, () -> remoteDirectory.listFilesByPrefixInLexicographicOrder("metadata", 1)); + } + public void testGetPendingDeletions() { assertThrows(UnsupportedOperationException.class, () -> remoteDirectory.getPendingDeletions()); } @@ -165,5 +219,4 @@ public void testRename() { public void testObtainLock() { assertThrows(UnsupportedOperationException.class, () -> remoteDirectory.obtainLock("segment_1")); } - } diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactoryTests.java index 324315505987b..bf4b2a14f2567 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactoryTests.java @@ -11,8 +11,11 @@ import org.apache.lucene.store.Directory; import org.junit.Before; import org.mockito.ArgumentCaptor; +import org.opensearch.action.ActionListener; +import org.opensearch.action.LatchedActionListener; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.blobstore.BlobContainer; +import org.opensearch.common.blobstore.BlobMetadata; import org.opensearch.common.blobstore.BlobPath; import org.opensearch.common.blobstore.BlobStore; import org.opensearch.common.settings.Settings; @@ -28,15 +31,16 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.Collections; import java.util.List; import java.util.function.Supplier; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.times; +import static org.mockito.Mockito.doAnswer; public class RemoteSegmentStoreDirectoryFactoryTests extends OpenSearchTestCase { @@ -68,7 +72,12 @@ public void testNewDirectory() throws IOException { when(repository.blobStore()).thenReturn(blobStore); when(repository.basePath()).thenReturn(new BlobPath().add("base_path")); when(blobStore.blobContainer(any())).thenReturn(blobContainer); - when(blobContainer.listBlobs()).thenReturn(Collections.emptyMap()); + doAnswer(invocation -> { + LatchedActionListener> latchedActionListener = invocation.getArgument(3); + latchedActionListener.onResponse(List.of()); + return null; + }).when(blobContainer) + .listBlobsByPrefixInSortedOrder(any(), eq(1), eq(BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC), any(ActionListener.class)); when(repositoriesService.repository("remote_store_repository")).thenReturn(repository); @@ -81,7 +90,12 @@ public void testNewDirectory() throws IOException { assertEquals("base_path/uuid_1/0/segments/metadata/", blobPaths.get(1).buildAsString()); assertEquals("base_path/uuid_1/0/segments/lock_files/", blobPaths.get(2).buildAsString()); - verify(blobContainer).listBlobsByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX); + verify(blobContainer).listBlobsByPrefixInSortedOrder( + eq(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX), + eq(1), + eq(BlobContainer.BlobNameSortOrder.LEXICOGRAPHIC), + any() + ); verify(repositoriesService, times(2)).repository("remote_store_repository"); } } diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java index 66e4b9a357b85..c37893877253e 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java @@ -32,6 +32,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.index.engine.NRTReplicationEngineFactory; +import org.opensearch.index.remote.RemoteStoreUtils; import org.opensearch.index.shard.IndexShard; import org.opensearch.index.shard.IndexShardTestCase; import org.opensearch.index.store.lockmanager.RemoteStoreMetadataLockManager; @@ -71,6 +72,10 @@ public class RemoteSegmentStoreDirectoryTests extends IndexShardTestCase { private SegmentInfos segmentInfos; private ThreadPool threadPool; + private final String metadataFilename = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(12, 23, 34, 1, 1); + private final String metadataFilename2 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(12, 13, 34, 1, 1); + private final String metadataFilename3 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(10, 38, 34, 1, 1); + @Before public void setup() throws IOException { remoteDataDirectory = mock(RemoteDirectory.class); @@ -119,50 +124,16 @@ public void testUploadedSegmentMetadataFromString() { assertEquals("_0.cfe::_0.cfe__uuidxyz::4567::372000", metadata.toString()); } - public void testGetMetadataFilename() { - // Generation 23 is replaced by n due to radix 32 - assertEquals( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX + "__12__n__uuid1", - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(12, 23, "uuid1") - ); - } - public void testGetPrimaryTermGenerationUuid() { - String[] filenameTokens = "abc__12__n__uuid_xyz".split(RemoteSegmentStoreDirectory.MetadataFilenameUtils.SEPARATOR); + String[] filenameTokens = "abc__9223372036854775795__9223372036854775784__uuid_xyz".split( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.SEPARATOR + ); assertEquals(12, RemoteSegmentStoreDirectory.MetadataFilenameUtils.getPrimaryTerm(filenameTokens)); assertEquals(23, RemoteSegmentStoreDirectory.MetadataFilenameUtils.getGeneration(filenameTokens)); - assertEquals("uuid_xyz", RemoteSegmentStoreDirectory.MetadataFilenameUtils.getUuid(filenameTokens)); - } - - public void testMetadataFilenameComparator() { - List metadataFilenames = new ArrayList<>( - List.of( - "abc__10__20__uuid1", - "abc__12__2__uuid2", - "pqr__1__1__uuid0", - "abc__3__n__uuid3", - "abc__10__8__uuid8", - "abc__3__a__uuid4", - "abc__3__a__uuid5" - ) - ); - metadataFilenames.sort(RemoteSegmentStoreDirectory.METADATA_FILENAME_COMPARATOR); - assertEquals( - List.of( - "abc__3__a__uuid4", - "abc__3__a__uuid5", - "abc__3__n__uuid3", - "abc__10__8__uuid8", - "abc__10__20__uuid1", - "abc__12__2__uuid2", - "pqr__1__1__uuid0" - ), - metadataFilenames - ); } public void testInitException() throws IOException { - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenThrow( + when(remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, 1)).thenThrow( new IOException("Error") ); @@ -262,29 +233,42 @@ private ByteArrayIndexInput createMetadataFileBytes(Map segmentF } private Map> populateMetadata() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc", "metadata__1__6__pqr", "metadata__2__1__zxv"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = new ArrayList<>(); + + metadataFiles.add(metadataFilename); + metadataFiles.add(metadataFilename2); + metadataFiles.add(metadataFilename3); + + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(List.of(metadataFilename)); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + Integer.MAX_VALUE + ) + ).thenReturn(metadataFiles); Map> metadataFilenameContentMapping = Map.of( - "metadata__1__5__abc", + metadataFilename, getDummyMetadata("_0", 1), - "metadata__1__6__pqr", + metadataFilename2, getDummyMetadata("_0", 1), - "metadata__2__1__zxv", + metadataFilename3, getDummyMetadata("_0", 1) ); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn( - createMetadataFileBytes(metadataFilenameContentMapping.get("metadata__1__5__abc"), 1, 5) + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenAnswer( + I -> createMetadataFileBytes(metadataFilenameContentMapping.get(metadataFilename), 23, 12) ); - when(remoteMetadataDirectory.openInput("metadata__1__6__pqr", IOContext.DEFAULT)).thenReturn( - createMetadataFileBytes(metadataFilenameContentMapping.get("metadata__1__6__pqr"), 1, 6) + when(remoteMetadataDirectory.openInput(metadataFilename2, IOContext.DEFAULT)).thenAnswer( + I -> createMetadataFileBytes(metadataFilenameContentMapping.get(metadataFilename2), 13, 12) ); - when(remoteMetadataDirectory.openInput("metadata__2__1__zxv", IOContext.DEFAULT)).thenReturn( - createMetadataFileBytes(metadataFilenameContentMapping.get("metadata__2__1__zxv"), 1, 2), - createMetadataFileBytes(metadataFilenameContentMapping.get("metadata__2__1__zxv"), 1, 2) + when(remoteMetadataDirectory.openInput(metadataFilename3, IOContext.DEFAULT)).thenAnswer( + I -> createMetadataFileBytes(metadataFilenameContentMapping.get(metadataFilename3), 38, 10) ); return metadataFilenameContentMapping; @@ -293,9 +277,12 @@ private Map> populateMetadata() throws IOException { public void testInit() throws IOException { populateMetadata(); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - List.of("metadata__1__5__abc", "metadata__1__6__pqr", "metadata__2__1__zxv") - ); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(List.of(metadataFilename)); remoteSegmentStoreDirectory.init(); @@ -399,15 +386,15 @@ public void testOpenInputException() throws IOException { public void testAcquireLock() throws IOException { populateMetadata(); remoteSegmentStoreDirectory.init(); - String mdFile = "xyz"; String acquirerId = "test-acquirer"; long testPrimaryTerm = 1; long testGeneration = 5; List metadataFiles = List.of("metadata__1__5__abc"); when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration), + 1 ) ).thenReturn(metadataFiles); @@ -437,8 +424,9 @@ public void testReleaseLock() throws IOException { List metadataFiles = List.of("metadata__1__5__abc"); when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration), + 1 ) ).thenReturn(metadataFiles); @@ -454,8 +442,9 @@ public void testIsAcquired() throws IOException { List metadataFiles = List.of("metadata__1__5__abc"); when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration), + 1 ) ).thenReturn(metadataFiles); @@ -471,8 +460,9 @@ public void testIsAcquiredException() throws IOException { List metadataFiles = new ArrayList<>(); when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration), + 1 ) ).thenReturn(metadataFiles); @@ -482,14 +472,10 @@ public void testIsAcquiredException() throws IOException { public void testGetMetadataFileForCommit() throws IOException { long testPrimaryTerm = 2; long testGeneration = 3; - List metadataFiles = List.of( - "metadata__1__5__abc", - "metadata__" + testPrimaryTerm + "__" + testGeneration + "__pqr", - "metadata__2__1__zxv" - ); when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration), + 1 ) ).thenReturn(List.of("metadata__" + testPrimaryTerm + "__" + testGeneration + "__pqr")); @@ -498,33 +484,6 @@ public void testGetMetadataFileForCommit() throws IOException { } - public void testGetSegmentsUploadedToRemoteStore() throws IOException { - long testPrimaryTerm = 1; - long testGeneration = 5; - - List metadataFiles = List.of("metadata__1__5__abc"); - when( - remoteMetadataDirectory.listFilesByPrefix( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilePrefixForCommit(testPrimaryTerm, testGeneration) - ) - ).thenReturn(metadataFiles); - - Map> metadataFilenameContentMapping = Map.of( - "metadata__1__5__abc", - getDummyMetadata("_0", 5), - "metadata__1__6__pqr", - getDummyMetadata("_0", 6), - "metadata__2__1__zxv", - getDummyMetadata("_0", 1) - ); - - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn( - createMetadataFileBytes(metadataFilenameContentMapping.get("metadata__1__5__abc"), 1, 5) - ); - - assert (remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStore(testPrimaryTerm, testGeneration).containsKey("segments_5")); - } - public void testCopyFrom() throws IOException { String filename = "_100.si"; populateMetadata(); @@ -556,46 +515,20 @@ public void testCopyFromException() throws IOException { storeDirectory.close(); } - public void testCopyFromOverride() throws IOException { - String filename = "_100.si"; - populateMetadata(); - remoteSegmentStoreDirectory.init(); - - Directory storeDirectory = LuceneTestCase.newDirectory(); - IndexOutput indexOutput = storeDirectory.createOutput(filename, IOContext.DEFAULT); - indexOutput.writeString("Hello World!"); - CodecUtil.writeFooter(indexOutput); - indexOutput.close(); - storeDirectory.sync(List.of(filename)); - - assertFalse(remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStore().containsKey(filename)); - remoteSegmentStoreDirectory.copyFrom(storeDirectory, filename, filename, IOContext.DEFAULT, true); - RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentStoreDirectory - .getSegmentsUploadedToRemoteStore() - .get(filename); - assertNotNull(uploadedSegmentMetadata); - remoteSegmentStoreDirectory.copyFrom(storeDirectory, filename, filename, IOContext.DEFAULT, true); - assertEquals( - uploadedSegmentMetadata.toString(), - remoteSegmentStoreDirectory.getSegmentsUploadedToRemoteStore().get(filename).toString() - ); - - storeDirectory.close(); - } - public void testContainsFile() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234::512"); metadata.put("_0.cfs", "_0.cfs::_0.cfs__" + UUIDs.base64UUID() + "::2345::1024"); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn( - createMetadataFileBytes(metadata, 1, 5) - ); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(createMetadataFileBytes(metadata, 1, 5)); remoteSegmentStoreDirectory.init(); @@ -625,7 +558,7 @@ public void testUploadMetadataEmpty() throws IOException { Collection segmentFiles = List.of("s1", "s2", "s3"); assertThrows( NoSuchFileException.class, - () -> remoteSegmentStoreDirectory.uploadMetadata(segmentFiles, segmentInfos, storeDirectory, 12L) + () -> remoteSegmentStoreDirectory.uploadMetadata(segmentFiles, segmentInfos, storeDirectory, 12L, 34L) ); } @@ -637,16 +570,19 @@ public void testUploadMetadataNonEmpty() throws IOException { BytesStreamOutput output = new BytesStreamOutput(); IndexOutput indexOutput = new OutputStreamIndexOutput("segment metadata", "metadata output stream", output, 4096); - long generation = segmentInfos.getGeneration(); - when(storeDirectory.createOutput(startsWith("metadata__12__" + generation), eq(IOContext.DEFAULT))).thenReturn(indexOutput); + String generation = RemoteStoreUtils.invertLong(segmentInfos.getGeneration()); + String primaryTerm = RemoteStoreUtils.invertLong(12); + when(storeDirectory.createOutput(startsWith("metadata__" + primaryTerm + "__" + generation), eq(IOContext.DEFAULT))).thenReturn( + indexOutput + ); Collection segmentFiles = List.of("_0.si", "_0.cfe", "_0.cfs", "segments_1"); - remoteSegmentStoreDirectory.uploadMetadata(segmentFiles, segmentInfos, storeDirectory, 12L); + remoteSegmentStoreDirectory.uploadMetadata(segmentFiles, segmentInfos, storeDirectory, 12L, 34L); verify(remoteMetadataDirectory).copyFrom( eq(storeDirectory), - startsWith("metadata__12__" + generation), - startsWith("metadata__12__" + generation), + startsWith("metadata__" + primaryTerm + "__" + generation), + startsWith("metadata__" + primaryTerm + "__" + generation), eq(IOContext.DEFAULT) ); @@ -669,10 +605,13 @@ public void testUploadMetadataNonEmpty() throws IOException { } public void testNoMetadataHeaderCorruptIndexException() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234"); @@ -683,16 +622,19 @@ public void testNoMetadataHeaderCorruptIndexException() throws IOException { indexOutput.writeMapOfStrings(metadata); indexOutput.close(); ByteArrayIndexInput byteArrayIndexInput = new ByteArrayIndexInput("segment metadata", BytesReference.toBytes(output.bytes())); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); assertThrows(CorruptIndexException.class, () -> remoteSegmentStoreDirectory.init()); } public void testInvalidCodecHeaderCorruptIndexException() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234"); @@ -705,16 +647,19 @@ public void testInvalidCodecHeaderCorruptIndexException() throws IOException { CodecUtil.writeFooter(indexOutput); indexOutput.close(); ByteArrayIndexInput byteArrayIndexInput = new ByteArrayIndexInput("segment metadata", BytesReference.toBytes(output.bytes())); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); assertThrows(CorruptIndexException.class, () -> remoteSegmentStoreDirectory.init()); } public void testHeaderMinVersionCorruptIndexException() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234"); @@ -727,16 +672,19 @@ public void testHeaderMinVersionCorruptIndexException() throws IOException { CodecUtil.writeFooter(indexOutput); indexOutput.close(); ByteArrayIndexInput byteArrayIndexInput = new ByteArrayIndexInput("segment metadata", BytesReference.toBytes(output.bytes())); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); assertThrows(IndexFormatTooOldException.class, () -> remoteSegmentStoreDirectory.init()); } public void testHeaderMaxVersionCorruptIndexException() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234"); @@ -749,16 +697,19 @@ public void testHeaderMaxVersionCorruptIndexException() throws IOException { CodecUtil.writeFooter(indexOutput); indexOutput.close(); ByteArrayIndexInput byteArrayIndexInput = new ByteArrayIndexInput("segment metadata", BytesReference.toBytes(output.bytes())); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); assertThrows(IndexFormatTooNewException.class, () -> remoteSegmentStoreDirectory.init()); } public void testIncorrectChecksumCorruptIndexException() throws IOException { - List metadataFiles = List.of("metadata__1__5__abc"); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenReturn( - metadataFiles - ); + List metadataFiles = List.of(metadataFilename); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + 1 + ) + ).thenReturn(metadataFiles); Map metadata = new HashMap<>(); metadata.put("_0.cfe", "_0.cfe::_0.cfe__" + UUIDs.base64UUID() + "::1234::512"); @@ -775,16 +726,19 @@ public void testIncorrectChecksumCorruptIndexException() throws IOException { indexOutputSpy.close(); ByteArrayIndexInput byteArrayIndexInput = new ByteArrayIndexInput("segment metadata", BytesReference.toBytes(output.bytes())); - when(remoteMetadataDirectory.openInput("metadata__1__5__abc", IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); + when(remoteMetadataDirectory.openInput(metadataFilename, IOContext.DEFAULT)).thenReturn(byteArrayIndexInput); assertThrows(CorruptIndexException.class, () -> remoteSegmentStoreDirectory.init()); } public void testDeleteStaleCommitsException() throws Exception { populateMetadata(); - when(remoteMetadataDirectory.listFilesByPrefix(RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX)).thenThrow( - new IOException("Error reading") - ); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + Integer.MAX_VALUE + ) + ).thenThrow(new IOException("Error reading")); // popluateMetadata() adds stub to return 3 metadata files // We are passing lastNMetadataFilesToKeep=2 here to validate that in case of exception deleteFile is not @@ -840,20 +794,20 @@ public void testDeleteStaleCommitsActualDelete() throws Exception { // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (String metadata : metadataFilenameContentMapping.get("metadata__1__5__abc").values()) { + for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; verify(remoteDataDirectory).deleteFile(uploadedFilename); } ; assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); - verify(remoteMetadataDirectory).deleteFile("metadata__1__5__abc"); + verify(remoteMetadataDirectory).deleteFile(metadataFilename3); } public void testDeleteStaleCommitsActualDeleteIOException() throws Exception { Map> metadataFilenameContentMapping = populateMetadata(); remoteSegmentStoreDirectory.init(); - String segmentFileWithException = metadataFilenameContentMapping.get("metadata__1__5__abc") + String segmentFileWithException = metadataFilenameContentMapping.get(metadataFilename3) .values() .stream() .findAny() @@ -864,20 +818,19 @@ public void testDeleteStaleCommitsActualDeleteIOException() throws Exception { // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (String metadata : metadataFilenameContentMapping.get("metadata__1__5__abc").values()) { + for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; verify(remoteDataDirectory).deleteFile(uploadedFilename); } - ; assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); - verify(remoteMetadataDirectory, times(0)).deleteFile("metadata__1__5__abc"); + verify(remoteMetadataDirectory, times(0)).deleteFile(metadataFilename3); } public void testDeleteStaleCommitsActualDeleteNoSuchFileException() throws Exception { Map> metadataFilenameContentMapping = populateMetadata(); remoteSegmentStoreDirectory.init(); - String segmentFileWithException = metadataFilenameContentMapping.get("metadata__1__5__abc") + String segmentFileWithException = metadataFilenameContentMapping.get(metadataFilename) .values() .stream() .findAny() @@ -888,13 +841,12 @@ public void testDeleteStaleCommitsActualDeleteNoSuchFileException() throws Excep // We are passing lastNMetadataFilesToKeep=2 here so that oldest 1 metadata file will be deleted remoteSegmentStoreDirectory.deleteStaleSegmentsAsync(2); - for (String metadata : metadataFilenameContentMapping.get("metadata__1__5__abc").values()) { + for (String metadata : metadataFilenameContentMapping.get(metadataFilename3).values()) { String uploadedFilename = metadata.split(RemoteSegmentStoreDirectory.UploadedSegmentMetadata.SEPARATOR)[1]; verify(remoteDataDirectory).deleteFile(uploadedFilename); } - ; assertBusy(() -> assertThat(remoteSegmentStoreDirectory.canDeleteStaleCommits.get(), is(true))); - verify(remoteMetadataDirectory).deleteFile("metadata__1__5__abc"); + verify(remoteMetadataDirectory).deleteFile(metadataFilename3); } public void testSegmentMetadataCurrentVersion() { @@ -909,6 +861,20 @@ public void testSegmentMetadataCurrentVersion() { assertEquals(RemoteSegmentMetadata.CURRENT_VERSION, 1); } + public void testMetadataFileNameOrder() { + String file1 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(15, 21, 23, 1, 1); + String file2 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(15, 38, 38, 1, 1); + String file3 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(18, 12, 26, 1, 1); + String file4 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(15, 38, 32, 10, 1); + String file5 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(15, 38, 32, 1, 1); + String file6 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename(15, 38, 32, 5, 1); + + List actualList = new ArrayList<>(List.of(file1, file2, file3, file4, file5, file6)); + actualList.sort(String::compareTo); + + assertEquals(List.of(file3, file2, file4, file6, file5, file1), actualList); + } + private static class WrapperIndexOutput extends IndexOutput { public IndexOutput indexOutput; diff --git a/server/src/test/java/org/opensearch/rest/NamedRouteTests.java b/server/src/test/java/org/opensearch/rest/NamedRouteTests.java index d489321ea5dc6..cf3e2b5b858bf 100644 --- a/server/src/test/java/org/opensearch/rest/NamedRouteTests.java +++ b/server/src/test/java/org/opensearch/rest/NamedRouteTests.java @@ -11,22 +11,17 @@ import org.opensearch.OpenSearchException; import org.opensearch.test.OpenSearchTestCase; +import java.util.Set; +import java.util.function.Function; + import static org.opensearch.rest.NamedRoute.MAX_LENGTH_OF_ACTION_NAME; +import static org.opensearch.rest.RestRequest.Method.GET; public class NamedRouteTests extends OpenSearchTestCase { - public void testNamedRouteWithNullName() { - try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", null); - fail("Expected NamedRoute to throw exception on null name provided"); - } catch (OpenSearchException e) { - assertTrue(e.getMessage().contains("Invalid route name specified")); - } - } - public void testNamedRouteWithEmptyName() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", ""); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("").build(); fail("Expected NamedRoute to throw exception on empty name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -35,7 +30,7 @@ public void testNamedRouteWithEmptyName() { public void testNamedRouteWithNameContainingSpace() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo bar"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo bar").build(); fail("Expected NamedRoute to throw exception on name containing space name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -44,7 +39,7 @@ public void testNamedRouteWithNameContainingSpace() { public void testNamedRouteWithNameContainingInvalidCharacters() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo@bar!"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo@bar!").build(); fail("Expected NamedRoute to throw exception on name containing invalid characters name provided"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -54,7 +49,7 @@ public void testNamedRouteWithNameContainingInvalidCharacters() { public void testNamedRouteWithNameOverMaximumLength() { try { String repeated = new String(new char[MAX_LENGTH_OF_ACTION_NAME + 1]).replace("\0", "x"); - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", repeated); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName(repeated).build(); fail("Expected NamedRoute to throw exception on name over maximum length supplied"); } catch (OpenSearchException e) { assertTrue(e.getMessage().contains("Invalid route name specified")); @@ -63,7 +58,7 @@ public void testNamedRouteWithNameOverMaximumLength() { public void testNamedRouteWithValidActionName() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -71,7 +66,7 @@ public void testNamedRouteWithValidActionName() { public void testNamedRouteWithValidActionNameWithForwardSlash() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/baz"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar:baz").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -79,7 +74,7 @@ public void testNamedRouteWithValidActionNameWithForwardSlash() { public void testNamedRouteWithValidActionNameWithWildcard() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar/*"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar/*").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } @@ -87,9 +82,82 @@ public void testNamedRouteWithValidActionNameWithWildcard() { public void testNamedRouteWithValidActionNameWithUnderscore() { try { - NamedRoute r = new NamedRoute(RestRequest.Method.GET, "foo/bar", "foo:bar_baz"); + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").build(); } catch (OpenSearchException e) { fail("Did not expect NamedRoute to throw exception on valid action name"); } } + + public void testNamedRouteWithNullLegacyActionNames() { + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar").legacyActionNames(null).build(); + assertTrue(r.actionNames().isEmpty()); + } catch (OpenSearchException e) { + fail("Did not expect NamedRoute to pass with an invalid legacy action name"); + } + } + + public void testNamedRouteWithInvalidLegacyActionNames() { + try { + NamedRoute r = new NamedRoute.Builder().method(GET) + .path("foo/bar") + .uniqueName("foo:bar") + .legacyActionNames(Set.of("foo:bar-legacy")) + .build(); + fail("Did not expect NamedRoute to pass with an invalid legacy action name"); + } catch (OpenSearchException e) { + assertTrue(e.getMessage().contains("Invalid action name [foo:bar-legacy]. It must start with one of:")); + } + } + + public void testNamedRouteWithHandler() { + Function fooHandler = restRequest -> null; + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").handler(fooHandler).build(); + assertEquals(r.handler(), fooHandler); + } catch (OpenSearchException e) { + fail("Did not expect NamedRoute to throw exception"); + } + } + + public void testNamedRouteNullChecks() { + try { + NamedRoute r = new NamedRoute.Builder().method(null).path("foo/bar").uniqueName("foo:bar_baz").build(); + fail("Expected NamedRoute to throw exception as method should not be null"); + } catch (NullPointerException e) { + assertEquals("REST method must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path(null).uniqueName("foo:bar_baz").build(); + fail("Expected NamedRoute to throw exception as path should not be null"); + } catch (NullPointerException e) { + assertEquals("REST path must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName(null).build(); + fail("Expected NamedRoute to throw exception as route name should not be null"); + } catch (NullPointerException e) { + assertEquals("REST route name must not be null.", e.getMessage()); + } + + try { + NamedRoute r = new NamedRoute.Builder().method(GET).path("foo/bar").uniqueName("foo:bar_baz").handler(null).build(); + fail("Expected NamedRoute to throw exception as handler should not be null"); + } catch (NullPointerException e) { + assertEquals("Route handler must not be null.", e.getMessage()); + } + } + + public void testNamedRouteEmptyBuild() { + try { + NamedRoute r = new NamedRoute.Builder().build(); + fail("Expected NamedRoute to throw exception as fields should not be null"); + } catch (IllegalStateException e) { + assertEquals("REST method, path and uniqueName are required.", e.getMessage()); + } + + } + } diff --git a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java index 7968c6c43afb4..df9cdd6669d23 100644 --- a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java +++ b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java @@ -45,7 +45,7 @@ public void testGetTracerWithTracingDisabledReturnsNoopTracer() { Tracer tracer = tracerFactory.getTracer(); assertTrue(tracer instanceof NoopTracer); - assertTrue(tracer.startSpan("foo") == Scope.NO_OP); + assertTrue(tracer.startSpan("foo") == SpanScope.NO_OP); } public void testGetTracerWithTracingEnabledReturnsDefaultTracer() { diff --git a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java index 4779d9796e11e..876145f6bf653 100644 --- a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java +++ b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockSpan.java @@ -142,6 +142,10 @@ public Long getEndTime() { return endTime; } + public void setError(Exception exception) { + putMetadata("ERROR", exception.getMessage()); + } + private static class IdGenerator { private static String generateSpanId() { long id = randomSupplier.get().nextLong();