From d40d8ebe8c755a5466053e32d4faa11801217ad1 Mon Sep 17 00:00:00 2001 From: Oleksandr Kolomiiets Date: Mon, 15 Apr 2024 13:58:48 -0700 Subject: [PATCH] Cleanup --- .../common/metrics/MetricAccessor.java | 54 --------------- .../util/ValueWithThreadLocalOverride.java | 67 +++++++++++++++++++ .../index/mapper/SourceLoader.java | 2 +- .../elasticsearch/indices/MapperMetrics.java | 20 ++---- .../index/mapper/SourceLoaderTests.java | 4 +- 5 files changed, 77 insertions(+), 70 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/common/metrics/MetricAccessor.java create mode 100644 server/src/main/java/org/elasticsearch/common/util/ValueWithThreadLocalOverride.java diff --git a/server/src/main/java/org/elasticsearch/common/metrics/MetricAccessor.java b/server/src/main/java/org/elasticsearch/common/metrics/MetricAccessor.java deleted file mode 100644 index 31f23e814a019..0000000000000 --- a/server/src/main/java/org/elasticsearch/common/metrics/MetricAccessor.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.common.metrics; - -import java.util.Objects; - -/** - * Provides easy access to metrics (or anything really) - * without the need to pass metrics object explicitly. - * Intended to use as a singleton in consumers but still allows injecting test values - * by first checking thread local value before falling back to set value. - */ -public class MetricAccessor { - // Intentionally not static - we expect users to only create instance - // per each "global" resource. - private ThreadLocal threadLocal = ThreadLocal.withInitial(() -> null); - private T value; - - public MetricAccessor(T value) { - this.value = Objects.requireNonNull(value); - } - - public AutoCloseable initForTest(T value) { - threadLocal.set(value); - - return new Reset(threadLocal); - } - - /** - * returns stored value or thread local value if set - * @return T - */ - public T get() { - var local = threadLocal.get(); - if (local != null) { - return local; - } - - return value; - } - - private record Reset(ThreadLocal threadLocal) implements AutoCloseable { - @Override - public void close() throws Exception { - threadLocal.remove(); - } - } -} diff --git a/server/src/main/java/org/elasticsearch/common/util/ValueWithThreadLocalOverride.java b/server/src/main/java/org/elasticsearch/common/util/ValueWithThreadLocalOverride.java new file mode 100644 index 0000000000000..9c3292dcffe31 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/util/ValueWithThreadLocalOverride.java @@ -0,0 +1,67 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.common.util; + +import java.util.function.Supplier; + +/** + * Stores value and provides optional functionality to set up a per-thread override value. + * Intended usage is a singleton value that is commonly accessed from multiple places. + * Having it as singleton allows to not pass instance down to every consumer. + * Thread-local override allows to use different value in tests even though it is a singleton. + * Inspired by tracing. + */ +public class ValueWithThreadLocalOverride { + // Intentionally not static - different values should allow different overrides. + private final ThreadLocal threadLocal = ThreadLocal.withInitial(() -> null); + private Supplier supplier; + + public ValueWithThreadLocalOverride(T value) { + this.supplier = () -> value; + } + + /** + * returns stored value or an override if set + * @return T + */ + public T get() { + return supplier.get(); + } + + /** + * Installs a thread-local override value. + * @param value + * @return an {@link AutoCloseable} that removes the override. + */ + public AutoCloseable withOverride(T value) { + threadLocal.set(value); + // This is a small optimization to eliminate thread local lookup + // if override was never set, which is most of the time. + T original = supplier.get(); + this.supplier = () -> getWithOverride(original); + + return new Reset(threadLocal); + } + + private T getWithOverride(T original) { + var local = threadLocal.get(); + if (local != null) { + return local; + } + + return original; + } + + private record Reset(ThreadLocal threadLocal) implements AutoCloseable { + @Override + public void close() throws Exception { + threadLocal.remove(); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java index 6beacb1933185..9fd99036b627f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java @@ -113,7 +113,7 @@ private record LeafWithMetrics(Leaf leaf) implements Leaf { @Override public Source source(LeafStoredFieldLoader storedFields, int docId) throws IOException { - SourceFieldMetrics metrics = MapperMetrics.INSTANCE.get().getSyntheticSourceMetrics(); + SourceFieldMetrics metrics = MapperMetrics.SOURCE_FIELD_METRICS.get(); long startTime = metrics.getRelativeTimeSupplier().getAsLong(); var source = leaf.source(storedFields, docId); diff --git a/server/src/main/java/org/elasticsearch/indices/MapperMetrics.java b/server/src/main/java/org/elasticsearch/indices/MapperMetrics.java index 9472fcdc72d95..be5dcc624f727 100644 --- a/server/src/main/java/org/elasticsearch/indices/MapperMetrics.java +++ b/server/src/main/java/org/elasticsearch/indices/MapperMetrics.java @@ -8,7 +8,7 @@ package org.elasticsearch.indices; -import org.elasticsearch.common.metrics.MetricAccessor; +import org.elasticsearch.common.util.ValueWithThreadLocalOverride; import org.elasticsearch.index.mapper.SourceFieldMetrics; /** @@ -16,21 +16,15 @@ * Main purpose of this class is to avoid verbosity of passing individual metric instances around. */ public class MapperMetrics { - public static MapperMetrics NOOP = new MapperMetrics(SourceFieldMetrics.NOOP); + public static MapperMetrics NOOP = new MapperMetrics(); - public static MetricAccessor INSTANCE = new MetricAccessor<>(NOOP); + public static ValueWithThreadLocalOverride SOURCE_FIELD_METRICS = new ValueWithThreadLocalOverride<>( + SourceFieldMetrics.NOOP + ); public static void init(SourceFieldMetrics sourceFieldMetrics) { - INSTANCE = new MetricAccessor<>(new MapperMetrics(sourceFieldMetrics)); + SOURCE_FIELD_METRICS = new ValueWithThreadLocalOverride<>(sourceFieldMetrics); } - private final SourceFieldMetrics sourceFieldMetrics; - - public MapperMetrics(SourceFieldMetrics sourceFieldMetrics) { - this.sourceFieldMetrics = sourceFieldMetrics; - } - - public SourceFieldMetrics getSyntheticSourceMetrics() { - return sourceFieldMetrics; - } + private MapperMetrics() {} } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/SourceLoaderTests.java b/server/src/test/java/org/elasticsearch/index/mapper/SourceLoaderTests.java index 255174ab5cfaf..891a26f69b848 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/SourceLoaderTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/SourceLoaderTests.java @@ -136,7 +136,7 @@ public void testHideTheCopyTo() { assertThat(e.getMessage(), equalTo("[copy_to] may not be used to copy from a multi-field: [foo.hidden]")); } - public void testSyntheticSourceTelemetry() throws Exception { + public void testSyntheticSourceMetrics() throws Exception { var testTelemetry = new TestTelemetryPlugin(); var sourceFieldMetrics = new SourceFieldMetrics( testTelemetry.getTelemetryProvider(Settings.EMPTY).getMeterRegistry(), @@ -153,7 +153,7 @@ public long getAsLong() { DocumentMapper mapper = createDocumentMapper( syntheticSourceMapping(b -> { b.startObject("kwd").field("type", "keyword").endObject(); }) ); - try (var ignored = MapperMetrics.INSTANCE.initForTest(new MapperMetrics(sourceFieldMetrics))) { + try (var ignored = MapperMetrics.SOURCE_FIELD_METRICS.withOverride(sourceFieldMetrics)) { assertThat(syntheticSource(mapper, b -> b.field("kwd", "foo")), equalTo(""" {"kwd":"foo"}"""));