From 5195cba24ea98521c9b5e63751145ca71012429f Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 14 Sep 2022 10:31:38 +0200 Subject: [PATCH 01/12] Fix ParentToChildrenAggregatorTests#testBestDeferringCollectorWithSubAggOfChildrenAggNeedingScores() test failures. (#90052) The failures reported in #90050 was caused by the fact that just a few docs were indexed and the string_field had in total just one value in the index. The second fix is that due test wrapping of index reader casts in ValueSource.java line 285 failed. A DirectoryReader is expected there, which is not the case if maybeWrap is true. Closes #90050 --- .../aggregations/ParentToChildrenAggregatorTests.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java index 1d3df776edecb..4fd958cf21d71 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java @@ -193,7 +193,9 @@ public void testBestDeferringCollectorWithSubAggOfChildrenAggNeedingScores() thr new ShardId(new Index("foo", "_na_"), 1) ) ) { - var indexSearcher = newSearcher(indexReader, true, false); + // maybeWrap should be false here, in ValueSource.java we sometimes cast to DirectoryReader and + // these casts can then fail if the maybeWrap is true. + var indexSearcher = newSearcher(indexReader, false, true); // invalid usage, { var aggregationBuilder = new ChildrenAggregationBuilder("_name1", CHILD_TYPE); @@ -246,8 +248,6 @@ public void testBestDeferringCollectorWithSubAggOfChildrenAggNeedingScores() thr Terms terms = result.getAggregations().get("_name2"); TopHits topHits = terms.getBuckets().get(0).getAggregations().get("_name3"); assertThat(topHits.getHits().getHits(), arrayWithSize(1)); - topHits = terms.getBuckets().get(1).getAggregations().get("_name3"); - assertThat(topHits.getHits().getHits(), arrayWithSize(1)); } } } @@ -298,7 +298,7 @@ private static List createChildDocument(String childId, String parentId, new StringField("join_field", CHILD_TYPE, Field.Store.NO), createJoinField(PARENT_TYPE, parentId), new SortedNumericDocValuesField("number", value), - new SortedSetDocValuesField("string_field", new BytesRef(randomBoolean() ? "1" : "2")) + new SortedSetDocValuesField("string_field", new BytesRef("str_value")) ); } From fde78111e6b137e1d6b859ff7e9b84ff69eb42f1 Mon Sep 17 00:00:00 2001 From: Salvatore Campagna <93581129+salvatore-campagna@users.noreply.github.com> Date: Wed, 14 Sep 2022 11:31:07 +0200 Subject: [PATCH 02/12] fix: use a delta value when comparing double values (#89963) --- .../search/aggregations/metrics/TDigestStateTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestStateTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestStateTests.java index ea60dbbafa068..a92de41dfc470 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestStateTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestStateTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matchers; import java.util.Arrays; @@ -33,7 +34,7 @@ public void testMoreThan4BValues() { for (double q : quantiles) { final double v = digest.quantile(q); logger.trace("q=" + q + ", v=" + v); - assertTrue(v >= prev); + assertThat(v, Matchers.either(Matchers.closeTo(prev, 0.0000001D)).or(Matchers.greaterThan(prev))); assertTrue("Unexpectedly low value: " + v, v >= 0.0); assertTrue("Unexpectedly high value: " + v, v <= 1.0); prev = v; From 07079366348d1e4a60b8152dff1b77511e4c0c3d Mon Sep 17 00:00:00 2001 From: Salvatore Campagna <93581129+salvatore-campagna@users.noreply.github.com> Date: Wed, 14 Sep 2022 11:31:27 +0200 Subject: [PATCH 03/12] fix: indexer state after call to stop() could be stopping or stopped (#89955) Here the test thread is calling two methods one after the other, stop and getState. The call to stop returns STOPPING after atomically updating state. Anyway, the call to getState might be interleaved with another thread which might (atomically) change the state of the indexer to STOPPED before the getState method is called by the test thread. Put it another way, calls to the stop method and calls to the getState method are not atomic but can interleave with another thread that (correctly) updates the state before the test thread is able to see the STOPPING state. A situation like the following might lead indeed to see STOPPED instead of STOPPING in the test thread: * thread1 calls stop() which returns STOPPING * thread2 updates state from STOPPING to STOPPED * thread1 calls getState which returns STOPPED (instead of STOPPING) --- .../xpack/rollup/job/RollupIndexerStateTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerStateTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerStateTests.java index e6ed75c29b7b4..6cba6bcfb5071 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerStateTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerStateTests.java @@ -31,6 +31,7 @@ import org.elasticsearch.xpack.core.rollup.job.RollupIndexerJobStats; import org.elasticsearch.xpack.core.rollup.job.RollupJob; import org.elasticsearch.xpack.core.rollup.job.RollupJobConfig; +import org.hamcrest.Matchers; import org.mockito.stubbing.Answer; import java.io.IOException; @@ -621,7 +622,7 @@ protected void onAbort() { final CountDownLatch latch = indexer.newLatch(); assertTrue(indexer.maybeTriggerAsyncJob(System.currentTimeMillis())); assertThat(indexer.stop(), equalTo(IndexerState.STOPPING)); - assertThat(indexer.getState(), equalTo(IndexerState.STOPPING)); + assertThat(indexer.getState(), Matchers.either(Matchers.is(IndexerState.STOPPING)).or(Matchers.is(IndexerState.STOPPED))); latch.countDown(); assertBusy(() -> assertThat(indexer.getState(), equalTo(IndexerState.STOPPED))); assertTrue(indexer.abort()); From d7b29fa9846da685927402995f00a6a66797be85 Mon Sep 17 00:00:00 2001 From: Iraklis Psaroudakis Date: Wed, 14 Sep 2022 15:21:30 +0300 Subject: [PATCH 04/12] Improve exception message on corrupted repository (#90034) After #89130 was solved, realized that this exception message can be now improved to mention to re-create the repository with the same settings. Co-authored-by: David Turner --- .../snapshots/ConcurrentSnapshotsIT.java | 5 +---- .../snapshots/CorruptedBlobStoreRepositoryIT.java | 5 +---- ...positoryIntegrityHealthIndicatorServiceIT.java | 2 +- .../blobstore/BlobStoreRepository.java | 15 +++++++++------ 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java index 68f5028c03a9b..9c656109dfd24 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java @@ -170,10 +170,7 @@ public void testRecreateCorruptedRepositoryDuringSnapshotsFails() throws Excepti final ExecutionException executionException = expectThrows(ExecutionException.class, () -> slowFuture.get().getSnapshotInfo()); final Throwable innermostException = Throwables.getRootCause(executionException); assertThat(innermostException, instanceOf(RepositoryException.class)); - assertThat( - innermostException.getMessage(), - containsString("Could not read repository data because the contents of the repository do not match its expected state") - ); + assertThat(innermostException.getMessage(), containsString("The repository has been disabled to prevent data corruption")); logger.info("--> without snapshots in progress, finally recreate repository to reset corrupted state"); createRepository(repoName, "mock", Settings.builder().put(repoSettings)); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/CorruptedBlobStoreRepositoryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/CorruptedBlobStoreRepositoryIT.java index 2dd217c117774..696e0de5d5578 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/CorruptedBlobStoreRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/CorruptedBlobStoreRepositoryIT.java @@ -829,10 +829,7 @@ private void assertRepositoryBlocked(String repo, String existingSnapshot) { RepositoryException.class, () -> clusterAdmin().prepareCreateSnapshot(repo, existingSnapshot).execute().actionGet() ); - assertThat( - ex2.getMessage(), - containsString("Could not read repository data because the contents of the repository do not match its expected state.") - ); + assertThat(ex2.getMessage(), containsString("The repository has been disabled to prevent data corruption")); logger.info("--> confirm corrupt flag in cluster state"); assertEquals(RepositoryData.CORRUPTED_REPO_GEN, getRepositoryMetadata(repo).generation()); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceIT.java index 2b7e86dbead6e..36ecee336712a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceIT.java @@ -58,7 +58,7 @@ public void testRepositoryIntegrityHealthIndicator() throws IOException, Interru // instead relies on other operations to detect and flag repository corruption assertThat( expectThrows(RepositoryException.class, () -> createFullSnapshot(repository, "snapshot-2")).getMessage(), - containsString("[" + repository + "] Could not read repository data") + containsString("[" + repository + "] The repository has been disabled to prevent data corruption") ); assertSnapshotRepositoryHealth("Indicator should be red after file is deleted from the repository", client, RED); diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index c0b5eec43070a..00035b6be72fd 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -2019,12 +2019,15 @@ private void cacheRepositoryData(RepositoryData repositoryData, Version version) private RepositoryException corruptedStateException(@Nullable Exception cause, @Nullable Tuple previousWriterInfo) { return new RepositoryException( metadata.name(), - "Could not read repository data because the contents of the repository do not match its " - + "expected state. This is likely the result of either concurrently modifying the contents of the " - + "repository by a process other than this cluster or an issue with the repository's underlying storage. " - + "The repository has been disabled to prevent corrupting its contents. To re-enable it " - + "and continue using it please remove the repository from the cluster and add it again to make " - + "the cluster recover the known state of the repository from its physical contents." + "The repository has been disabled to prevent data corruption because its contents were found not to match its expected state. " + + "This is either because something other than this cluster modified the repository contents, or because the repository's " + + "underlying storage behaves incorrectly. To re-enable this repository, first ensure that this cluster has exclusive " + + "write access to it, and then re-register the repository with this cluster. " + + "See https://www.elastic.co/guide/en/elasticsearch/reference/" + + Version.CURRENT.major + + "." + + Version.CURRENT.minor + + "/add-repository.html for further information." + previousWriterMessage(previousWriterInfo), cause ); From aed64a6c763ccedde89fa875a64e4147b0111121 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 14 Sep 2022 15:32:17 +0100 Subject: [PATCH 05/12] Add a TSID global ordinal to TimeSeriesIndexSearcher (#90035) Rather than trying to compare BytesRefs in tsdb-related aggregations, it will be much quicker if we can use a search-global ordinal to detect when we have moved to a new TSID. This commit adds such an ordinal to the aggregation execution context. --- docs/changelog/90035.yaml | 5 +++++ .../aggregations/ParentJoinAggregator.java | 4 +++- .../AggregationExecutionContext.java | 19 ++++++++++++++----- .../search/aggregations/BucketCollector.java | 2 +- .../timeseries/TimeSeriesAggregator.java | 2 +- .../timeseries/TimeSeriesIndexSearcher.java | 12 ++++++++---- .../MultiBucketCollectorTests.java | 2 +- .../BestBucketsDeferringCollectorTests.java | 2 +- .../bucket/filter/FilterAggregatorTests.java | 2 +- .../bucket/filter/FiltersAggregatorTests.java | 2 +- .../TimeSeriesIndexSearcherTests.java | 16 +++++++++++----- .../topmetrics/TopMetricsAggregatorTests.java | 2 +- 12 files changed, 48 insertions(+), 22 deletions(-) create mode 100644 docs/changelog/90035.yaml diff --git a/docs/changelog/90035.yaml b/docs/changelog/90035.yaml new file mode 100644 index 0000000000000..2120071d592e5 --- /dev/null +++ b/docs/changelog/90035.yaml @@ -0,0 +1,5 @@ +pr: 90035 +summary: Add a TSID global ordinal to `TimeSeriesIndexSearcher` +area: TSDB +type: feature +issues: [] diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java index 48d9e8305e09d..9c6a788ea2f77 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java @@ -122,7 +122,9 @@ protected void prepareSubAggs(long[] ordsToCollect) throws IOException { continue; } DocIdSetIterator childDocsIter = childDocsScorer.iterator(); - final LeafBucketCollector sub = collectableSubAggregators.getLeafCollector(new AggregationExecutionContext(ctx, null, null)); + final LeafBucketCollector sub = collectableSubAggregators.getLeafCollector( + new AggregationExecutionContext(ctx, null, null, null) + ); final SortedSetDocValues globalOrdinals = valuesSource.globalOrdinalsValues(ctx); // Set the scorer, since we now replay only the child docIds diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java index f51ec04be535d..88a78a512d1bd 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java @@ -11,6 +11,8 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.util.BytesRef; +import java.util.function.IntSupplier; +import java.util.function.LongSupplier; import java.util.function.Supplier; /** @@ -21,18 +23,21 @@ */ public class AggregationExecutionContext { - private final Supplier tsidProvider; - private final Supplier timestampProvider; + private final Supplier tsidProvider; // TODO remove this entirely? + private final LongSupplier timestampProvider; + private final IntSupplier tsidOrdProvider; private final LeafReaderContext leafReaderContext; public AggregationExecutionContext( LeafReaderContext leafReaderContext, Supplier tsidProvider, - Supplier timestampProvider + LongSupplier timestampProvider, + IntSupplier tsidOrdProvider ) { this.leafReaderContext = leafReaderContext; this.tsidProvider = tsidProvider; this.timestampProvider = timestampProvider; + this.tsidOrdProvider = tsidOrdProvider; } public LeafReaderContext getLeafReaderContext() { @@ -43,7 +48,11 @@ public BytesRef getTsid() { return tsidProvider != null ? tsidProvider.get() : null; } - public Long getTimestamp() { - return timestampProvider.get(); + public long getTimestamp() { + return timestampProvider.getAsLong(); + } + + public int getTsidOrd() { + return tsidOrdProvider.getAsInt(); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/BucketCollector.java b/server/src/main/java/org/elasticsearch/search/aggregations/BucketCollector.java index 87052b6cf54ac..69e60a6f5d6cd 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/BucketCollector.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/BucketCollector.java @@ -83,7 +83,7 @@ private record BucketCollectorWrapper(BucketCollector bucketCollector) implement @Override public LeafCollector getLeafCollector(LeafReaderContext context) throws IOException { - return bucketCollector.getLeafCollector(new AggregationExecutionContext(context, null, null)); + return bucketCollector.getLeafCollector(new AggregationExecutionContext(context, null, null, null)); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesAggregator.java index 4b1786d608da1..8a077b100e1ea 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesAggregator.java @@ -42,7 +42,7 @@ public TimeSeriesAggregator( CardinalityUpperBound bucketCardinality, Map metadata ) throws IOException { - super(name, factories, context, parent, bucketCardinality, metadata); + super(name, factories, context, parent, CardinalityUpperBound.MANY, metadata); this.keyed = keyed; bucketOrds = BytesKeyedBucketOrds.build(bigArrays(), bucketCardinality); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java index 55edb863af5e8..5dfcf489c7760 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcher.java @@ -33,6 +33,7 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.function.IntSupplier; import static org.elasticsearch.index.IndexSortConfig.TIME_SERIES_SORT; @@ -65,6 +66,7 @@ public void search(Query query, BucketCollector bucketCollector) throws IOExcept int seen = 0; query = searcher.rewrite(query); Weight weight = searcher.createWeight(query, bucketCollector.scoreMode(), 1); + int[] tsidOrd = new int[1]; // Create LeafWalker for each subreader List leafWalkers = new ArrayList<>(); @@ -74,7 +76,7 @@ public void search(Query query, BucketCollector bucketCollector) throws IOExcept } Scorer scorer = weight.scorer(leaf); if (scorer != null) { - LeafWalker leafWalker = new LeafWalker(leaf, scorer, bucketCollector, leaf); + LeafWalker leafWalker = new LeafWalker(leaf, scorer, bucketCollector, () -> tsidOrd[0]); if (leafWalker.nextDoc() != DocIdSetIterator.NO_MORE_DOCS) { leafWalkers.add(leafWalker); } @@ -83,7 +85,7 @@ public void search(Query query, BucketCollector bucketCollector) throws IOExcept // this is needed to trigger actions in some bucketCollectors that bypass the normal iteration logic // for example, global aggregator triggers a separate iterator that ignores the query but still needs // to know all leaves - bucketCollector.getLeafCollector(new AggregationExecutionContext(leaf, null, null)); + bucketCollector.getLeafCollector(new AggregationExecutionContext(leaf, null, null, null)); } } @@ -115,6 +117,7 @@ protected boolean lessThan(LeafWalker a, LeafWalker b) { queue.updateTop(); } } while (queue.size() > 0); + tsidOrd[0]++; } } @@ -178,8 +181,9 @@ private static class LeafWalker { int tsidOrd; long timestamp; - LeafWalker(LeafReaderContext context, Scorer scorer, BucketCollector bucketCollector, LeafReaderContext leaf) throws IOException { - AggregationExecutionContext aggCtx = new AggregationExecutionContext(leaf, scratch::get, () -> timestamp); + LeafWalker(LeafReaderContext context, Scorer scorer, BucketCollector bucketCollector, IntSupplier tsidOrdSupplier) + throws IOException { + AggregationExecutionContext aggCtx = new AggregationExecutionContext(context, scratch::get, () -> timestamp, tsidOrdSupplier); this.collector = bucketCollector.getLeafCollector(aggCtx); liveDocs = context.reader().getLiveDocs(); this.collector.setScorer(scorer); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/MultiBucketCollectorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/MultiBucketCollectorTests.java index 7f244bfaf2431..cfb9c4bb83249 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/MultiBucketCollectorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/MultiBucketCollectorTests.java @@ -213,7 +213,7 @@ public void testNotTerminated() throws IOException { for (Map.Entry expectedCount : expectedCounts.entrySet()) { shouldNoop &= expectedCount.getValue().intValue() <= expectedCount.getKey().getTotalHits(); } - LeafBucketCollector collector = wrapped.getLeafCollector(new AggregationExecutionContext(ctx, null, null)); + LeafBucketCollector collector = wrapped.getLeafCollector(new AggregationExecutionContext(ctx, null, null, null)); assertThat(collector.isNoop(), equalTo(shouldNoop)); if (false == collector.isNoop()) { for (int docId = 0; docId < ctx.reader().numDocs(); docId++) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollectorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollectorTests.java index 2f9b8a1fd9e92..3d8c5b306d889 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollectorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollectorTests.java @@ -210,7 +210,7 @@ public ScoreMode scoreMode() { @Override public LeafBucketCollector getLeafCollector(LeafReaderContext context) throws IOException { LeafBucketCollector delegate = deferringCollector.getLeafCollector( - new AggregationExecutionContext(context, null, null) + new AggregationExecutionContext(context, null, null, null) ); return leafCollector.apply(deferringCollector, delegate); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java index 86ac87363463b..5cbce4a198fde 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java @@ -111,7 +111,7 @@ public void testBucketComparator() throws IOException { FilterAggregator agg = createAggregator(builder, indexSearcher, fieldType); agg.preCollection(); LeafBucketCollector collector = agg.getLeafCollector( - new AggregationExecutionContext(indexReader.leaves().get(0), null, null) + new AggregationExecutionContext(indexReader.leaves().get(0), null, null, null) ); collector.collect(0, 0); collector.collect(0, 0); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java index dc7797ef9301b..f2095ceab870c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java @@ -1633,7 +1633,7 @@ protected List objectMappers() { private Map collectAndGetFilterDebugInfo(IndexSearcher searcher, Aggregator aggregator) throws IOException { aggregator.preCollection(); for (LeafReaderContext ctx : searcher.getIndexReader().leaves()) { - LeafBucketCollector leafCollector = aggregator.getLeafCollector(new AggregationExecutionContext(ctx, null, null)); + LeafBucketCollector leafCollector = aggregator.getLeafCollector(new AggregationExecutionContext(ctx, null, null, null)); assertTrue(leafCollector.isNoop()); } Map debug = new HashMap<>(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java index 4553ee4230084..c103b7dc63488 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/timeseries/TimeSeriesIndexSearcherTests.java @@ -43,6 +43,7 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.elasticsearch.index.IndexSortConfig.TIME_SERIES_SORT; +import static org.hamcrest.Matchers.greaterThan; public class TimeSeriesIndexSearcherTests extends ESTestCase { @@ -175,9 +176,10 @@ private RandomIndexWriter getIndexWriter(Directory dir) throws IOException { private BucketCollector getBucketCollector(long totalCount) { return new BucketCollector() { - boolean tsidReverse = TIME_SERIES_SORT[0].getOrder() == SortOrder.DESC; - boolean timestampReverse = TIME_SERIES_SORT[1].getOrder() == SortOrder.DESC; + final boolean tsidReverse = TIME_SERIES_SORT[0].getOrder() == SortOrder.DESC; + final boolean timestampReverse = TIME_SERIES_SORT[1].getOrder() == SortOrder.DESC; BytesRef currentTSID = null; + int currentTSIDord = -1; long currentTimestamp = 0; long total = 0; @@ -197,7 +199,7 @@ public void collect(int doc, long owningBucketOrd) throws IOException { BytesRef latestTSID = tsid.lookupOrd(tsid.ordValue()); long latestTimestamp = timestamp.longValue(); assertEquals(latestTSID, aggCtx.getTsid()); - assertEquals(latestTimestamp, aggCtx.getTimestamp().longValue()); + assertEquals(latestTimestamp, aggCtx.getTimestamp()); if (currentTSID != null) { assertTrue( @@ -209,22 +211,26 @@ public void collect(int doc, long owningBucketOrd) throws IOException { currentTimestamp + "->" + latestTimestamp, timestampReverse ? latestTimestamp <= currentTimestamp : latestTimestamp >= currentTimestamp ); + assertEquals(currentTSIDord, aggCtx.getTsidOrd()); + } else { + assertThat(aggCtx.getTsidOrd(), greaterThan(currentTSIDord)); } } currentTimestamp = latestTimestamp; currentTSID = BytesRef.deepCopyOf(latestTSID); + currentTSIDord = aggCtx.getTsidOrd(); total++; } }; } @Override - public void preCollection() throws IOException { + public void preCollection() { } @Override - public void postCollection() throws IOException { + public void postCollection() { assertEquals(totalCount, total); } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorTests.java index 77c8fd7e27c8e..d6cd9d2b36617 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorTests.java @@ -357,7 +357,7 @@ public void testTonsOfBucketsTriggersBreaker() throws IOException { aggregator.preCollection(); assertThat(indexReader.leaves(), hasSize(1)); LeafBucketCollector leaf = aggregator.getLeafCollector( - new AggregationExecutionContext(indexReader.leaves().get(0), null, null) + new AggregationExecutionContext(indexReader.leaves().get(0), null, null, null) ); /* From a23999e7006d1f5577b4e0a9b1d723ce5d9461fc Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Wed, 14 Sep 2022 10:42:59 -0400 Subject: [PATCH 06/12] Support DoubleValues expression scripts in lang-expression (#89895) This allows for custom DoubleValues scripts to be made with lang-expressions. The change also adds a way for plugin integration and unit tests to be able to create class loaders, after this permission was removed in 8.0. --- docs/changelog/89895.yaml | 5 ++ .../ExpressionDoubleValuesScript.java | 88 +++++++++++++++++++ .../expression/ExpressionScriptEngine.java | 9 ++ .../ExpressionDoubleValuesScriptTests.java | 87 ++++++++++++++++++ .../script/DoubleValuesScript.java | 46 ++++++++++ .../elasticsearch/script/ScriptModule.java | 3 +- .../plugins/PluginsServiceTests.java | 16 ++++ .../test/PrivilegedOperations.java | 9 ++ 8 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 docs/changelog/89895.yaml create mode 100644 modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScript.java create mode 100644 modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScriptTests.java create mode 100644 server/src/main/java/org/elasticsearch/script/DoubleValuesScript.java diff --git a/docs/changelog/89895.yaml b/docs/changelog/89895.yaml new file mode 100644 index 0000000000000..4c53ac118a8d8 --- /dev/null +++ b/docs/changelog/89895.yaml @@ -0,0 +1,5 @@ +pr: 89895 +summary: Initial code to support binary expression scripts +area: "Infra/Scripting" +type: enhancement +issues: [] diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScript.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScript.java new file mode 100644 index 0000000000000..a7935800ec4ba --- /dev/null +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScript.java @@ -0,0 +1,88 @@ +/* + * 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.script.expression; + +import org.apache.lucene.expressions.Bindings; +import org.apache.lucene.expressions.Expression; +import org.apache.lucene.search.DoubleValues; +import org.apache.lucene.search.DoubleValuesSource; +import org.apache.lucene.search.Rescorer; +import org.apache.lucene.search.SortField; +import org.elasticsearch.script.DoubleValuesScript; + +import java.util.function.Function; + +/** + * A factory for a custom compiled {@link Expression} scripts + *

+ * Instead of an execution result, we return a wrapper to an {@link Expression} object, which + * can be used for all supported double values operations. + */ +public class ExpressionDoubleValuesScript implements DoubleValuesScript.Factory { + private final Expression exprScript; + + ExpressionDoubleValuesScript(Expression e) { + this.exprScript = e; + } + + @Override + public DoubleValuesScript newInstance() { + return new DoubleValuesScript() { + @Override + public double execute() { + return exprScript.evaluate(new DoubleValues[0]); + } + + @Override + public double evaluate(DoubleValues[] functionValues) { + return exprScript.evaluate(functionValues); + } + + @Override + public DoubleValuesSource getDoubleValuesSource(Function sourceProvider) { + return exprScript.getDoubleValuesSource(new Bindings() { + @Override + public DoubleValuesSource getDoubleValuesSource(String name) { + return sourceProvider.apply(name); + } + }); + } + + @Override + public SortField getSortField(Function sourceProvider, boolean reverse) { + return exprScript.getSortField(new Bindings() { + @Override + public DoubleValuesSource getDoubleValuesSource(String name) { + return sourceProvider.apply(name); + } + }, reverse); + } + + @Override + public Rescorer getRescorer(Function sourceProvider) { + return exprScript.getRescorer(new Bindings() { + @Override + public DoubleValuesSource getDoubleValuesSource(String name) { + return sourceProvider.apply(name); + } + }); + } + + @Override + public String sourceText() { + return exprScript.sourceText; + } + + @Override + public String[] variables() { + return exprScript.variables; + } + }; + } +} diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java index a6c62b10b9f81..7870421817466 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java @@ -24,6 +24,7 @@ import org.elasticsearch.script.BucketAggregationScript; import org.elasticsearch.script.BucketAggregationSelectorScript; import org.elasticsearch.script.ClassPermission; +import org.elasticsearch.script.DoubleValuesScript; import org.elasticsearch.script.FieldScript; import org.elasticsearch.script.FilterScript; import org.elasticsearch.script.NumberSortScript; @@ -132,6 +133,14 @@ public FieldScript.LeafFactory newFactory(Map params, SearchLook return newFieldScript(expr, lookup, params); } + @Override + public boolean isResultDeterministic() { + return true; + } + }, + + DoubleValuesScript.CONTEXT, + (Expression expr) -> new ExpressionDoubleValuesScript(expr) { @Override public boolean isResultDeterministic() { return true; diff --git a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScriptTests.java b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScriptTests.java new file mode 100644 index 0000000000000..bce884a422871 --- /dev/null +++ b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/ExpressionDoubleValuesScriptTests.java @@ -0,0 +1,87 @@ +/* + * 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.script.expression; + +import org.apache.lucene.expressions.SimpleBindings; +import org.apache.lucene.search.DoubleValues; +import org.apache.lucene.search.DoubleValuesSource; +import org.apache.lucene.search.SortField; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.script.DoubleValuesScript; +import org.elasticsearch.script.Script; +import org.elasticsearch.script.ScriptException; +import org.elasticsearch.script.ScriptModule; +import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.ScriptType; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.text.ParseException; +import java.util.Collections; +import java.util.Map; + +/** + * Tests {@link ExpressionDoubleValuesScript} through the {@link ScriptService} + */ +public class ExpressionDoubleValuesScriptTests extends ESTestCase { + private ExpressionScriptEngine engine; + private ScriptService scriptService; + + @Override + public void setUp() throws Exception { + super.setUp(); + + engine = new ExpressionScriptEngine(); + scriptService = new ScriptService(Settings.EMPTY, Map.of("expression", engine), ScriptModule.CORE_CONTEXTS, () -> 1L); + } + + @SuppressWarnings("unchecked") + private DoubleValuesScript compile(String expression) { + var script = new Script(ScriptType.INLINE, "expression", expression, Collections.emptyMap()); + return scriptService.compile(script, DoubleValuesScript.CONTEXT).newInstance(); + } + + public void testCompileError() { + ScriptException e = expectThrows(ScriptException.class, () -> compile("10 * log(10)")); + assertTrue(e.getCause() instanceof ParseException); + assertEquals("Invalid expression '10 * log(10)': Unrecognized function call (log).", e.getCause().getMessage()); + } + + public void testEvaluate() { + var expression = compile("10 * log10(10)"); + assertEquals("10 * log10(10)", expression.sourceText()); + assertEquals(10.0, expression.evaluate(new DoubleValues[0]), 0.00001); + assertEquals(10.0, expression.execute(), 0.00001); + + expression = compile("20 * log10(a)"); + assertEquals("20 * log10(a)", expression.sourceText()); + assertEquals(20.0, expression.evaluate(new DoubleValues[] { DoubleValues.withDefault(DoubleValues.EMPTY, 10.0) }), 0.00001); + } + + public void testDoubleValuesSource() throws IOException { + SimpleBindings bindings = new SimpleBindings(); + bindings.add("popularity", DoubleValuesSource.constant(5)); + + var expression = compile("10 * log10(popularity)"); + var doubleValues = expression.getDoubleValuesSource((name) -> bindings.getDoubleValuesSource(name)); + assertEquals("expr(10 * log10(popularity))", doubleValues.toString()); + var values = doubleValues.getValues(null, null); + assertTrue(values.advanceExact(0)); + assertEquals(6, (int) values.doubleValue()); + + var sortField = expression.getSortField((name) -> bindings.getDoubleValuesSource(name), false); + assertEquals("expr(10 * log10(popularity))", sortField.getField()); + assertEquals(SortField.Type.CUSTOM, sortField.getType()); + assertFalse(sortField.getReverse()); + + var rescorer = expression.getRescorer((name) -> bindings.getDoubleValuesSource(name)); + assertNotNull(rescorer); + } + +} diff --git a/server/src/main/java/org/elasticsearch/script/DoubleValuesScript.java b/server/src/main/java/org/elasticsearch/script/DoubleValuesScript.java new file mode 100644 index 0000000000000..a540f2c77906b --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/DoubleValuesScript.java @@ -0,0 +1,46 @@ +/* + * 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.script; + +import org.apache.lucene.search.DoubleValues; +import org.apache.lucene.search.DoubleValuesSource; +import org.apache.lucene.search.Rescorer; +import org.apache.lucene.search.SortField; + +import java.util.function.Function; + +/** + * A custom script that can be used for various DoubleValue Lucene operations. + */ +public abstract class DoubleValuesScript { + + public DoubleValuesScript() {} + + public abstract double execute(); + + public abstract double evaluate(DoubleValues[] functionValues); + + public abstract DoubleValuesSource getDoubleValuesSource(Function sourceProvider); + + public abstract SortField getSortField(Function sourceProvider, boolean reverse); + + public abstract Rescorer getRescorer(Function sourceProvider); + + public abstract String sourceText(); + + public abstract String[] variables(); + + /** A factory to construct {@link DoubleValuesScript} instances. */ + public interface Factory extends ScriptFactory { + DoubleValuesScript newInstance(); + } + + @SuppressWarnings("rawtypes") + public static final ScriptContext CONTEXT = new ScriptContext<>("double_values", Factory.class); +} diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java index 097f8aa5d6d1d..996d6815aa433 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -68,7 +68,8 @@ public class ScriptModule { ScriptedMetricAggContexts.MapScript.CONTEXT, ScriptedMetricAggContexts.CombineScript.CONTEXT, ScriptedMetricAggContexts.ReduceScript.CONTEXT, - IntervalFilterScript.CONTEXT + IntervalFilterScript.CONTEXT, + DoubleValuesScript.CONTEXT ), RUNTIME_FIELDS_CONTEXTS.stream() ).collect(Collectors.toMap(c -> c.name, Function.identity())); diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index afc1a5c5a101b..cb4d4ad6262dd 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -33,6 +33,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.security.AccessControlException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -766,6 +767,21 @@ public Map getElectionStrategies() { } } + public void testCanCreateAClassLoader() { + assertEquals( + "access denied (\"java.lang.RuntimePermission\" \"createClassLoader\")", + expectThrows(AccessControlException.class, () -> new Loader(this.getClass().getClassLoader())).getMessage() + ); + var loader = PrivilegedOperations.supplierWithCreateClassLoader(() -> new Loader(this.getClass().getClassLoader())); + assertEquals(this.getClass().getClassLoader(), loader.getParent()); + } + + static final class Loader extends ClassLoader { + Loader(ClassLoader parent) { + super(parent); + } + } + // Closes the URLClassLoaders of plugins loaded by the given plugin service. static void closePluginLoaders(PluginsService pluginService) { for (var lp : pluginService.plugins()) { diff --git a/test/framework/src/main/java/org/elasticsearch/test/PrivilegedOperations.java b/test/framework/src/main/java/org/elasticsearch/test/PrivilegedOperations.java index fd85470ef39e6..2c19a81e09a71 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/PrivilegedOperations.java +++ b/test/framework/src/main/java/org/elasticsearch/test/PrivilegedOperations.java @@ -59,6 +59,15 @@ public static Boolean compilationTaskCall(JavaCompiler.CompilationTask compilati ); } + public static T supplierWithCreateClassLoader(Supplier supplier) { + return AccessController.doPrivileged( + (PrivilegedAction) () -> supplier.get(), + context, + new RuntimePermission("createClassLoader"), + new RuntimePermission("closeClassLoader") + ); + } + @SuppressForbidden(reason = "need to create file permission") private static FilePermission newAllFilesReadPermission() { return new FilePermission("<>", "read"); From cc0679ea81201001f55d55129a206ec4bb00c657 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Wed, 14 Sep 2022 08:46:58 -0700 Subject: [PATCH 07/12] Fix Fields API Caching Regression (#90017) This fixes both a bug and a performance regression. After adding the text mappings family to the scripting fields API, field-style access uses source to generate values while doc-style access uses doc values. This means that a script accessing the same text field using both apis may see incorrect results. There is also a performance regression where previously we checked a single cache to try to ensure that doc-style access only used doc values, but this was done on a per-document basis. This change adds separate caches for field-style access and doc-style access in LeafDocLookup where all the work to cache field data is done per-segment, and the parallel caches allow no additional checks to be made when accessing the values per-document. The caches still share field data when possible for non-text based fields, so we don't double load. --- docs/changelog/90017.yaml | 5 + .../painless/45_script_doc_values_cache.yml | 191 +++++++++++ .../search/lookup/LeafDocLookup.java | 134 ++++++-- .../search/lookup/LeafDocLookupTests.java | 302 ++++++++++++++++++ 4 files changed, 607 insertions(+), 25 deletions(-) create mode 100644 docs/changelog/90017.yaml create mode 100644 modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/45_script_doc_values_cache.yml diff --git a/docs/changelog/90017.yaml b/docs/changelog/90017.yaml new file mode 100644 index 0000000000000..a827809e87b9e --- /dev/null +++ b/docs/changelog/90017.yaml @@ -0,0 +1,5 @@ +pr: 90017 +summary: Fix Fields API Caching Regression +area: Infra/Scripting +type: regression +issues: [] diff --git a/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/45_script_doc_values_cache.yml b/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/45_script_doc_values_cache.yml new file mode 100644 index 0000000000000..9dffeab747319 --- /dev/null +++ b/modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/45_script_doc_values_cache.yml @@ -0,0 +1,191 @@ +setup: + - do: + indices.create: + index: test0 + body: + settings: + number_of_shards: 1 + mappings: + properties: + text: + type: text + fielddata: true + long: + type: long + + - do: + index: + index: test0 + id: "1" + body: + text: "Lots of text." + long: 1 + + - do: + indices.create: + index: test1 + body: + settings: + number_of_shards: 1 + mappings: + properties: + text: + type: text + fielddata: true + long: + type: long + + - do: + index: + index: test1 + id: "1" + body: + text: "Lots of text." + long: 1 + + - do: + indices.refresh: {} + +--- +"test_leaf_cache_text_field_first": + - do: + search: + rest_total_hits_as_int: true + index: test0 + body: + script_fields: + field_0: + script: + source: "/* avoid stash */ $('text', '') + ' ' + doc['text'].value" + - match: { hits.hits.0.fields.field_0.0: 'Lots of text. lots' } + + - do: + search: + rest_total_hits_as_int: true + index: test0 + body: + script_fields: + field_0: + script: + source: "/* avoid stash */ $('text', '') + ' ' + doc['text'].value" + field_1: + script: + source: "doc['text'].value + ' ' + $('text', '')" + field_2: + script: + source: "/* avoid stash */ $('text', '') + ' ' + doc['text'].value" + field_3: + script: + source: "doc['text'].value + ' ' + $('text', '')" + - match: { hits.hits.0.fields.field_0.0: 'Lots of text. lots' } + - match: { hits.hits.0.fields.field_1.0: 'lots Lots of text.' } + - match: { hits.hits.0.fields.field_2.0: 'Lots of text. lots' } + - match: { hits.hits.0.fields.field_3.0: 'lots Lots of text.' } + +--- +"test_leaf_cache_text_doc_first": + - do: + search: + rest_total_hits_as_int: true + index: test1 + body: + script_fields: + field_0: + script: + source: "doc['text'].value + ' ' + $('text', '')" + - match: { hits.hits.0.fields.field_0.0: 'lots Lots of text.' } + + - do: + search: + rest_total_hits_as_int: true + index: test1 + body: + script_fields: + field_0: + script: + source: "doc['text'].value + ' ' + $('text', '')" + field_1: + script: + source: "/* avoid stash */ $('text', '') + ' ' + doc['text'].value" + field_2: + script: + source: "doc['text'].value + ' ' + $('text', '')" + field_3: + script: + source: "/* avoid stash */ $('text', '') + ' ' + doc['text'].value" + - match: { hits.hits.0.fields.field_0.0: 'lots Lots of text.' } + - match: { hits.hits.0.fields.field_1.0: 'Lots of text. lots' } + - match: { hits.hits.0.fields.field_2.0: 'lots Lots of text.' } + - match: { hits.hits.0.fields.field_3.0: 'Lots of text. lots' } + +--- +"test_leaf_cache_long_field_first": + - do: + search: + rest_total_hits_as_int: true + index: test0 + body: + script_fields: + field_0: + script: + source: "/* avoid stash */ $('long', 0) + ' ' + doc['long'].value" + - match: { hits.hits.0.fields.field_0.0: '1 1' } + + - do: + search: + rest_total_hits_as_int: true + index: test0 + body: + script_fields: + field_0: + script: + source: "/* avoid stash */ $('long', 0) + ' ' + doc['long'].value" + field_1: + script: + source: "doc['long'].value + ' ' + $('long', 0)" + field_2: + script: + source: "/* avoid stash */ $('long', 0) + ' ' + doc['long'].value" + field_3: + script: + source: "doc['long'].value + ' ' + $('long', 0)" + - match: { hits.hits.0.fields.field_0.0: '1 1' } + - match: { hits.hits.0.fields.field_1.0: '1 1' } + - match: { hits.hits.0.fields.field_2.0: '1 1' } + - match: { hits.hits.0.fields.field_3.0: '1 1' } + +--- +"test_leaf_cache_long_doc_first": + - do: + search: + rest_total_hits_as_int: true + index: test1 + body: + script_fields: + field_0: + script: + source: "doc['long'].value + ' ' + $('long', 0)" + - match: { hits.hits.0.fields.field_0.0: '1 1' } + + - do: + search: + rest_total_hits_as_int: true + index: test1 + body: + script_fields: + field_0: + script: + source: "doc['long'].value + ' ' + $('long', 0)" + field_1: + script: + source: "/* avoid stash */ $('long', 0) + ' ' + doc['long'].value" + field_2: + script: + source: "doc['long'].value + ' ' + $('long', 0)" + field_3: + script: + source: "/* avoid stash */ $('long', 0) + ' ' + doc['long'].value" + - match: { hits.hits.0.fields.field_0.0: '1 1' } + - match: { hits.hits.0.fields.field_1.0: '1 1' } + - match: { hits.hits.0.fields.field_2.0: '1 1' } + - match: { hits.hits.0.fields.field_3.0: '1 1' } diff --git a/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java b/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java index 93fbfc1a5b43c..e188f8a9fde60 100644 --- a/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java +++ b/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java @@ -26,6 +26,9 @@ import java.util.function.BiFunction; import java.util.function.Function; +import static org.elasticsearch.index.mapper.MappedFieldType.FielddataOperation.SCRIPT; +import static org.elasticsearch.index.mapper.MappedFieldType.FielddataOperation.SEARCH; + public class LeafDocLookup implements Map> { private final Function fieldTypeLookup; @@ -34,7 +37,20 @@ public class LeafDocLookup implements Map> { private int docId = -1; - private final Map localCacheScriptFieldData = Maps.newMapWithExpectedSize(4); + /* + We run parallel caches for the fields-access API ( field('f') ) and + the doc-access API.( doc['f'] ) for two reasons: + 1. correctness - the field cache can store fields that retrieve values + from both doc values and source whereas the doc cache + can only store doc values. This leads to cases such as text + field where sharing a cache could lead to incorrect results in a + script that uses both types of access (likely common during upgrades) + 2. performance - to keep the performance reasonable we move all caching updates to + per-segment computation as opposed to per-document computation + Note that we share doc values between both caches when possible. + */ + final Map fieldFactoryCache = Maps.newMapWithExpectedSize(4); + final Map docFactoryCache = Maps.newMapWithExpectedSize(4); LeafDocLookup( Function fieldTypeLookup, @@ -50,32 +66,48 @@ public void setDocument(int docId) { this.docId = docId; } - protected DocValuesScriptFieldFactory getScriptFieldFactory(String fieldName, MappedFieldType.FielddataOperation options) { - DocValuesScriptFieldFactory factory = localCacheScriptFieldData.get(fieldName); + // used to load data for a field-style api accessor + private DocValuesScriptFieldFactory getFactoryForField(String fieldName) { + final MappedFieldType fieldType = fieldTypeLookup.apply(fieldName); - // do not use cached source fallback fields for old style doc access - if (options == MappedFieldType.FielddataOperation.SEARCH - && factory instanceof SourceValueFetcherIndexFieldData.ValueFetcherDocValues) { - factory = null; + if (fieldType == null) { + throw new IllegalArgumentException("No field found for [" + fieldName + "] in mapping"); } - if (factory == null) { - final MappedFieldType fieldType = fieldTypeLookup.apply(fieldName); + // Load the field data on behalf of the script. Otherwise, it would require + // additional permissions to deal with pagedbytes/ramusagestimator/etc. + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public DocValuesScriptFieldFactory run() { + DocValuesScriptFieldFactory fieldFactory = null; + IndexFieldData indexFieldData = fieldDataLookup.apply(fieldType, SCRIPT); - if (fieldType == null) { - throw new IllegalArgumentException("No field found for [" + fieldName + "] in mapping"); - } + DocValuesScriptFieldFactory docFactory = null; + + if (docFactoryCache.isEmpty() == false) { + docFactory = docFactoryCache.get(fieldName); + } - // Load the field data on behalf of the script. Otherwise, it would require - // additional permissions to deal with pagedbytes/ramusagestimator/etc. - factory = AccessController.doPrivileged(new PrivilegedAction() { - @Override - public DocValuesScriptFieldFactory run() { - return fieldDataLookup.apply(fieldType, options).load(reader).getScriptFieldFactory(fieldName); + // if this field has already been accessed via the doc-access API and the field-access API + // uses doc values then we share to avoid double-loading + if (docFactory != null && indexFieldData instanceof SourceValueFetcherIndexFieldData == false) { + fieldFactory = docFactory; + } else { + fieldFactory = indexFieldData.load(reader).getScriptFieldFactory(fieldName); } - }); - localCacheScriptFieldData.put(fieldName, factory); + fieldFactoryCache.put(fieldName, fieldFactory); + + return fieldFactory; + } + }); + } + + public Field getScriptField(String fieldName) { + DocValuesScriptFieldFactory factory = fieldFactoryCache.get(fieldName); + + if (factory == null) { + factory = getFactoryForField(fieldName); } try { @@ -84,22 +116,74 @@ public DocValuesScriptFieldFactory run() { throw ExceptionsHelper.convertToElastic(ioe); } - return factory; + return factory.toScriptField(); } - public Field getScriptField(String fieldName) { - return getScriptFieldFactory(fieldName, MappedFieldType.FielddataOperation.SCRIPT).toScriptField(); + // used to load data for a doc-style api accessor + private DocValuesScriptFieldFactory getFactoryForDoc(String fieldName) { + final MappedFieldType fieldType = fieldTypeLookup.apply(fieldName); + + if (fieldType == null) { + throw new IllegalArgumentException("No field found for [" + fieldName + "] in mapping"); + } + + // Load the field data on behalf of the script. Otherwise, it would require + // additional permissions to deal with pagedbytes/ramusagestimator/etc. + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public DocValuesScriptFieldFactory run() { + DocValuesScriptFieldFactory docFactory = null; + IndexFieldData indexFieldData = fieldDataLookup.apply(fieldType, SEARCH); + + DocValuesScriptFieldFactory fieldFactory = null; + + if (fieldFactoryCache.isEmpty() == false) { + fieldFactory = fieldFactoryCache.get(fieldName); + } + + if (fieldFactory != null) { + IndexFieldData fieldIndexFieldData = fieldDataLookup.apply(fieldType, SCRIPT); + + // if this field has already been accessed via the field-access API and the field-access API + // uses doc values then we share to avoid double-loading + if (fieldIndexFieldData instanceof SourceValueFetcherIndexFieldData == false) { + docFactory = fieldFactory; + } + } + + if (docFactory == null) { + docFactory = indexFieldData.load(reader).getScriptFieldFactory(fieldName); + } + + docFactoryCache.put(fieldName, docFactory); + + return docFactory; + } + }); } @Override public ScriptDocValues get(Object key) { - return getScriptFieldFactory(key.toString(), MappedFieldType.FielddataOperation.SEARCH).toScriptDocValues(); + String fieldName = key.toString(); + DocValuesScriptFieldFactory factory = docFactoryCache.get(fieldName); + + if (factory == null) { + factory = getFactoryForDoc(key.toString()); + } + + try { + factory.setNextDocId(docId); + } catch (IOException ioe) { + throw ExceptionsHelper.convertToElastic(ioe); + } + + return factory.toScriptDocValues(); } @Override public boolean containsKey(Object key) { String fieldName = key.toString(); - return localCacheScriptFieldData.get(fieldName) != null || fieldTypeLookup.apply(fieldName) != null; + return docFactoryCache.containsKey(key) || fieldFactoryCache.containsKey(key) || fieldTypeLookup.apply(fieldName) != null; } @Override diff --git a/server/src/test/java/org/elasticsearch/search/lookup/LeafDocLookupTests.java b/server/src/test/java/org/elasticsearch/search/lookup/LeafDocLookupTests.java index 7c8921e9123a0..e951242b253fb 100644 --- a/server/src/test/java/org/elasticsearch/search/lookup/LeafDocLookupTests.java +++ b/server/src/test/java/org/elasticsearch/search/lookup/LeafDocLookupTests.java @@ -10,17 +10,23 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.LeafFieldData; import org.elasticsearch.index.fielddata.ScriptDocValues; +import org.elasticsearch.index.fielddata.SourceValueFetcherIndexFieldData; import org.elasticsearch.index.mapper.DynamicFieldType; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperBuilderContext; import org.elasticsearch.index.mapper.flattened.FlattenedFieldMapper; import org.elasticsearch.script.field.DelegateDocValuesField; +import org.elasticsearch.script.field.DocValuesScriptFieldFactory; +import org.elasticsearch.script.field.Field; import org.elasticsearch.test.ESTestCase; import org.junit.Before; import java.io.IOException; +import java.util.Map; import java.util.function.BiFunction; +import static org.elasticsearch.index.mapper.MappedFieldType.FielddataOperation.SCRIPT; +import static org.elasticsearch.index.mapper.MappedFieldType.FielddataOperation.SEARCH; import static org.mockito.AdditionalAnswers.returnsFirstArg; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; @@ -112,4 +118,300 @@ public void setNextDocId(int id) { return fieldData; } + + public void testParallelCache() { + String nameDoc = "doc"; // field where search and script return doc values + String nameSource = "source"; // field where search returns no data and script returns source values + String nameDocAndSource = "docAndSource"; // field where search returns doc values and script returns source values + + MappedFieldType docMappedFieldType = mock(MappedFieldType.class); + MappedFieldType sourceMappedFieldType = mock(MappedFieldType.class); + MappedFieldType docAndSourceMappedFieldType = mock(MappedFieldType.class); + + Map namesToMappedFieldTypes = Map.of( + nameDoc, + docMappedFieldType, + nameSource, + sourceMappedFieldType, + nameDocAndSource, + docAndSourceMappedFieldType + ); + + IndexFieldData docIndexFieldData = mock(IndexFieldData.class); + SourceValueFetcherIndexFieldData sourceIndexFieldData = mock(SourceValueFetcherIndexFieldData.class); + IndexFieldData docAndSourceDocIndexFieldData = mock(IndexFieldData.class); + SourceValueFetcherIndexFieldData docAndSourceSourceIndexFieldData = mock(SourceValueFetcherIndexFieldData.class); + + LeafFieldData docLeafFieldData = mock(LeafFieldData.class); + LeafFieldData sourceLeafFieldData = mock(SourceValueFetcherIndexFieldData.SourceValueFetcherLeafFieldData.class); + LeafFieldData docAndSourceDocLeafFieldData = mock(LeafFieldData.class); + LeafFieldData docAndSourceSourceLeafFieldData = mock(SourceValueFetcherIndexFieldData.SourceValueFetcherLeafFieldData.class); + + DocValuesScriptFieldFactory docFactory = mock(DocValuesScriptFieldFactory.class); + DocValuesScriptFieldFactory sourceFactory = mock(DocValuesScriptFieldFactory.class); + DocValuesScriptFieldFactory docAndSourceDocFactory = mock(DocValuesScriptFieldFactory.class); + DocValuesScriptFieldFactory docAndSourceSourceFactory = mock(DocValuesScriptFieldFactory.class); + + ScriptDocValues docDocValues = mock(ScriptDocValues.class); + Field fieldDocValues = mock(Field.class); + Field fieldSourceValues = mock(Field.class); + ScriptDocValues docSourceAndDocValues = mock(ScriptDocValues.class); + Field fieldSourceAndDocValues = mock(Field.class); + + doReturn(docLeafFieldData).when(docIndexFieldData).load(any()); + doReturn(docFactory).when(docLeafFieldData).getScriptFieldFactory(nameDoc); + doReturn(docDocValues).when(docFactory).toScriptDocValues(); + doReturn(fieldDocValues).when(docFactory).toScriptField(); + + doReturn(sourceLeafFieldData).when(sourceIndexFieldData).load(any()); + doReturn(sourceFactory).when(sourceLeafFieldData).getScriptFieldFactory(nameSource); + doReturn(fieldSourceValues).when(sourceFactory).toScriptField(); + + doReturn(docAndSourceDocLeafFieldData).when(docAndSourceDocIndexFieldData).load(any()); + doReturn(docAndSourceDocFactory).when(docAndSourceDocLeafFieldData).getScriptFieldFactory(nameDocAndSource); + doReturn(docSourceAndDocValues).when(docAndSourceDocFactory).toScriptDocValues(); + + doReturn(docAndSourceSourceLeafFieldData).when(docAndSourceSourceIndexFieldData).load(any()); + doReturn(docAndSourceSourceFactory).when(docAndSourceSourceLeafFieldData).getScriptFieldFactory(nameDocAndSource); + doReturn(fieldSourceAndDocValues).when(docAndSourceSourceFactory).toScriptField(); + + LeafDocLookup leafDocLookup = new LeafDocLookup(namesToMappedFieldTypes::get, (mappedFieldType, operation) -> { + if (mappedFieldType.equals(docMappedFieldType)) { + if (operation == SEARCH || operation == SCRIPT) { + return docIndexFieldData; + } else { + throw new IllegalArgumentException("unknown operation [" + operation + "]"); + } + } else if (mappedFieldType.equals(sourceMappedFieldType)) { + if (operation == SEARCH) { + throw new IllegalArgumentException("search cannot access source"); + } else if (operation == SCRIPT) { + return sourceIndexFieldData; + } else { + throw new IllegalArgumentException("unknown operation [" + operation + "]"); + } + } else if (mappedFieldType.equals(docAndSourceMappedFieldType)) { + if (operation == SEARCH) { + return docAndSourceDocIndexFieldData; + } else if (operation == SCRIPT) { + return docAndSourceSourceIndexFieldData; + } else { + throw new IllegalArgumentException("unknown operation [" + operation + "]"); + } + } else { + throw new IllegalArgumentException("unknown mapped field type [" + mappedFieldType + "]"); + } + }, null); + + // load shared doc values field into cache w/ doc-access first + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertTrue(leafDocLookup.fieldFactoryCache.isEmpty()); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // load shared doc values field into cache w/ field-access first + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertTrue(leafDocLookup.docFactoryCache.isEmpty()); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // load source values field into cache + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + expectThrows(IllegalArgumentException.class, () -> leafDocLookup.get(nameSource)); + assertTrue(leafDocLookup.docFactoryCache.isEmpty()); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // load doc values for doc-access and script values for script-access from the same index field data w/ doc-access first + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertTrue(leafDocLookup.fieldFactoryCache.isEmpty()); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // load doc values for doc-access and script values for script-access from the same index field data w/ field-access first + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertTrue(leafDocLookup.docFactoryCache.isEmpty()); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(1, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(1, leafDocLookup.docFactoryCache.size()); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // add all 3 fields to the cache w/ doc-access first + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + // clear the cache + leafDocLookup.docFactoryCache.clear(); + leafDocLookup.fieldFactoryCache.clear(); + + // add all 3 fields to the cache w/ field-access first + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + + assertEquals(fieldDocValues, leafDocLookup.getScriptField(nameDoc)); + assertEquals(fieldSourceValues, leafDocLookup.getScriptField(nameSource)); + assertEquals(fieldSourceAndDocValues, leafDocLookup.getScriptField(nameDocAndSource)); + assertEquals(docDocValues, leafDocLookup.get(nameDoc)); + assertEquals(docSourceAndDocValues, leafDocLookup.get(nameDocAndSource)); + assertEquals(3, leafDocLookup.fieldFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.fieldFactoryCache.get(nameDoc)); + assertEquals(sourceFactory, leafDocLookup.fieldFactoryCache.get(nameSource)); + assertEquals(docAndSourceSourceFactory, leafDocLookup.fieldFactoryCache.get(nameDocAndSource)); + assertEquals(2, leafDocLookup.docFactoryCache.size()); + assertEquals(docFactory, leafDocLookup.docFactoryCache.get(nameDoc)); + assertEquals(docAndSourceDocFactory, leafDocLookup.docFactoryCache.get(nameDocAndSource)); + } } From f262f36564e9786dd3ea2138a22f42d13fcb4753 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 14 Sep 2022 12:23:46 -0400 Subject: [PATCH 08/12] Add master_timeout to the snapshot delete docs (#90032) --- .../snapshot-restore/apis/delete-snapshot-api.asciidoc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/reference/snapshot-restore/apis/delete-snapshot-api.asciidoc b/docs/reference/snapshot-restore/apis/delete-snapshot-api.asciidoc index a060e7f2d59df..5bc46f54ec137 100644 --- a/docs/reference/snapshot-restore/apis/delete-snapshot-api.asciidoc +++ b/docs/reference/snapshot-restore/apis/delete-snapshot-api.asciidoc @@ -53,6 +53,11 @@ Name of the repository to delete a snapshot from. (Required, string) Comma-separated list of snapshot names to delete. Also accepts wildcards (`*`). +[[delete-snapshot-api-query-params]] +==== {api-query-parms-title} + +include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=master-timeout] + [[delete-snapshot-api-example]] ==== {api-example-title} From 8f591e72f8afa4fcb434ba61ebc3492258f11b0e Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Wed, 14 Sep 2022 13:31:03 -0500 Subject: [PATCH 09/12] Adding node name to cluster_formation section of stable master health API results (#89982) --- docs/reference/health/health.asciidoc | 3 ++ .../StableMasterHealthIndicatorService.java | 40 ++++++++++++++++--- .../java/org/elasticsearch/node/Node.java | 2 +- ...ableMasterHealthIndicatorServiceTests.java | 21 ++++++---- .../AbstractCoordinatorTestCase.java | 2 +- 5 files changed, 53 insertions(+), 15 deletions(-) diff --git a/docs/reference/health/health.asciidoc b/docs/reference/health/health.asciidoc index c3fdb5aa42114..8ed467656dc4f 100644 --- a/docs/reference/health/health.asciidoc +++ b/docs/reference/health/health.asciidoc @@ -273,6 +273,9 @@ details have contents and a structure that is unique to each indicator. `node_id`:: (string) The node id of a master-eligible node +`name`:: +(Optional, string) The node name of a master-eligible node + `cluster_formation_message`:: (string) A detailed description explaining what went wrong with cluster formation, or why this node was unable to join the cluster if it has formed. diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java b/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java index ceec6ced7ae6d..aff14a5039d5b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java @@ -9,6 +9,8 @@ package org.elasticsearch.cluster.coordination; import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.core.Nullable; import org.elasticsearch.health.Diagnosis; import org.elasticsearch.health.HealthIndicatorDetails; import org.elasticsearch.health.HealthIndicatorImpact; @@ -20,6 +22,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Objects; /** * This indicator reports the health of master stability. @@ -48,6 +51,7 @@ public class StableMasterHealthIndicatorService implements HealthIndicatorServic ); private final CoordinationDiagnosticsService coordinationDiagnosticsService; + private final ClusterService clusterService; // Keys for the details map: private static final String DETAILS_CURRENT_MASTER = "current_master"; @@ -73,8 +77,12 @@ public class StableMasterHealthIndicatorService implements HealthIndicatorServic new HealthIndicatorImpact(3, UNSTABLE_MASTER_BACKUP_IMPACT, List.of(ImpactArea.BACKUP)) ); - public StableMasterHealthIndicatorService(CoordinationDiagnosticsService coordinationDiagnosticsService) { + public StableMasterHealthIndicatorService( + CoordinationDiagnosticsService coordinationDiagnosticsService, + ClusterService clusterService + ) { this.coordinationDiagnosticsService = coordinationDiagnosticsService; + this.clusterService = clusterService; } @Override @@ -159,17 +167,37 @@ private HealthIndicatorDetails getDetails( if (coordinationDiagnosticsDetails.nodeToClusterFormationDescriptionMap() != null) { builder.field( CLUSTER_FORMATION, - coordinationDiagnosticsDetails.nodeToClusterFormationDescriptionMap() - .entrySet() - .stream() - .map(entry -> Map.of("node_id", entry.getKey(), CLUSTER_FORMATION_MESSAGE, entry.getValue())) - .toList() + coordinationDiagnosticsDetails.nodeToClusterFormationDescriptionMap().entrySet().stream().map(entry -> { + String nodeName = getNameForNodeId(entry.getKey()); + if (nodeName == null) { + return Map.of("node_id", entry.getKey(), CLUSTER_FORMATION_MESSAGE, entry.getValue()); + } else { + return Map.of("node_id", entry.getKey(), "name", nodeName, CLUSTER_FORMATION_MESSAGE, entry.getValue()); + } + }).toList() ); } return builder.endObject(); }; } + /** + * Returns the name of the node with the given nodeId, as seen in the cluster state at this moment. The name of a node is optional, + * so if the node does not have a name (or the node with the given nodeId is no longer in the cluster state), null is returned. + * @param nodeId The id of the node whose name is to be returned + * @return The current name of the node, or null if the node is not in the cluster state or does not have a name + */ + @Nullable + private String getNameForNodeId(String nodeId) { + DiscoveryNode node = clusterService.state().nodes().get(nodeId); + if (node == null) { + return null; + } else { + String nodeName = node.getName(); + return Objects.requireNonNullElse(nodeName, null); + } + } + /** * This method returns the only user action that is relevant when the master is unstable -- contact support. * @param explain If true, the returned list includes a UserAction to contact support, otherwise an empty list diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 7c6bb0f6f1b0d..3ab3444420254 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/server/src/main/java/org/elasticsearch/node/Node.java @@ -1162,7 +1162,7 @@ private HealthService createHealthService( CoordinationDiagnosticsService coordinationDiagnosticsService ) { List preflightHealthIndicatorServices = Collections.singletonList( - new StableMasterHealthIndicatorService(coordinationDiagnosticsService) + new StableMasterHealthIndicatorService(coordinationDiagnosticsService, clusterService) ); var serverHealthIndicatorServices = List.of( new RepositoryIntegrityHealthIndicatorService(clusterService), diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java index 7e17266765e90..48300520fcd92 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java @@ -137,12 +137,16 @@ public void testGetHealthIndicatorResultNotGreenExplainTrue() throws Exception { } List> clusterFormations = (List>) detailsMap.get("cluster_formation"); assertThat(clusterFormations.size(), equalTo(2)); - Map nodeToClusterFormationMap = new HashMap<>(); + Map nodeIdToClusterFormationMap = new HashMap<>(); + Map nodeIdToNodeNameMap = new HashMap<>(); for (Map clusterFormationMap : clusterFormations) { - nodeToClusterFormationMap.put(clusterFormationMap.get("node_id"), clusterFormationMap.get("cluster_formation_message")); + nodeIdToClusterFormationMap.put(clusterFormationMap.get("node_id"), clusterFormationMap.get("cluster_formation_message")); + nodeIdToNodeNameMap.put(clusterFormationMap.get("node_id"), clusterFormationMap.get("name")); } - assertThat(nodeToClusterFormationMap.get(node1.getId()), equalTo(node1ClusterFormation)); - assertThat(nodeToClusterFormationMap.get(node2.getId()), equalTo(node2ClusterFormation)); + assertThat(nodeIdToClusterFormationMap.get(node1.getId()), equalTo(node1ClusterFormation)); + assertThat(nodeIdToClusterFormationMap.get(node2.getId()), equalTo(node2ClusterFormation)); + assertThat(nodeIdToNodeNameMap.get(node1.getId()), equalTo(node1.getName())); + assertThat(nodeIdToNodeNameMap.get(node2.getId()), equalTo(node2.getName())); List diagnosis = result.diagnosisList(); assertThat(diagnosis.size(), equalTo(1)); assertThat(diagnosis.get(0), is(StableMasterHealthIndicatorService.CONTACT_SUPPORT_USER_ACTION)); @@ -259,14 +263,16 @@ public void testCalculate() throws Exception { } - private static ClusterState createClusterState(DiscoveryNode masterNode) { + private ClusterState createClusterState(DiscoveryNode masterNode) { var routingTableBuilder = RoutingTable.builder(); Metadata.Builder metadataBuilder = Metadata.builder(); DiscoveryNodes.Builder nodesBuilder = DiscoveryNodes.builder(); if (masterNode != null) { nodesBuilder.masterNodeId(masterNode.getId()); - nodesBuilder.add(masterNode); } + nodesBuilder.add(node1); + nodesBuilder.add(node2); + nodesBuilder.add(node3); return ClusterState.builder(new ClusterName("test-cluster")) .routingTable(routingTableBuilder.build()) .metadata(metadataBuilder.build()) @@ -309,7 +315,8 @@ private static StableMasterHealthIndicatorService createStableMasterHealthIndica when(coordinator.getFoundPeers()).thenReturn(Collections.emptyList()); TransportService transportService = mock(TransportService.class); return new StableMasterHealthIndicatorService( - new CoordinationDiagnosticsService(clusterService, transportService, coordinator, masterHistoryService) + new CoordinationDiagnosticsService(clusterService, transportService, coordinator, masterHistoryService), + clusterService ); } diff --git a/test/framework/src/main/java/org/elasticsearch/cluster/coordination/AbstractCoordinatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/cluster/coordination/AbstractCoordinatorTestCase.java index 0f4df5fd9b442..fb04fc2ca6c51 100644 --- a/test/framework/src/main/java/org/elasticsearch/cluster/coordination/AbstractCoordinatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/cluster/coordination/AbstractCoordinatorTestCase.java @@ -1281,7 +1281,7 @@ public RecyclerBytesStreamOutput newNetworkBytesStream() { null, getNamedWriteableRegistry() ); - stableMasterHealthIndicatorService = new StableMasterHealthIndicatorService(coordinationDiagnosticsService); + stableMasterHealthIndicatorService = new StableMasterHealthIndicatorService(coordinationDiagnosticsService, clusterService); masterService.setClusterStatePublisher(coordinator); final GatewayService gatewayService = new GatewayService( settings, From 9cbdc2a8733a83439adc4a720a94bac07b1f1bf6 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Wed, 14 Sep 2022 14:12:41 -0500 Subject: [PATCH 10/12] Updating HealthService to use FetchHealthInfoCacheAction (#89947) This PR updates HealthService to fetch HealthInfo data using FetchHealthInfoCacheAction (#89820), and to pass it to the HealthIndicatorServices' calculate method. This will allow HealthIndicatorServices to use any data from the health node's HealthInfoCache. --- .../health/GetHealthActionIT.java | 3 +- .../elasticsearch/health/HealthServiceIT.java | 169 ++++++++++++ .../StableMasterHealthIndicatorService.java | 3 +- ...rdsAvailabilityHealthIndicatorService.java | 3 +- .../common/settings/ClusterSettings.java | 4 +- .../elasticsearch/health/GetHealthAction.java | 23 +- .../health/HealthIndicatorService.java | 4 +- .../elasticsearch/health/HealthService.java | 99 +++++-- .../elasticsearch/health/node/HealthInfo.java | 2 + .../action/TransportHealthNodeAction.java | 30 ++- ...sitoryIntegrityHealthIndicatorService.java | 3 +- ...ableMasterHealthIndicatorServiceTests.java | 3 +- ...ailabilityHealthIndicatorServiceTests.java | 29 ++- .../health/HealthIndicatorServiceTests.java | 3 +- .../health/HealthServiceTests.java | 246 ++++++++++++++++-- ...yIntegrityHealthIndicatorServiceTests.java | 7 +- .../xpack/ilm/IlmHealthIndicatorService.java | 3 +- .../xpack/slm/SlmHealthIndicatorService.java | 3 +- .../ilm/IlmHealthIndicatorServiceTests.java | 9 +- .../slm/SlmHealthIndicatorServiceTests.java | 13 +- 20 files changed, 572 insertions(+), 87 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/elasticsearch/health/HealthServiceIT.java diff --git a/server/src/internalClusterTest/java/org/elasticsearch/health/GetHealthActionIT.java b/server/src/internalClusterTest/java/org/elasticsearch/health/GetHealthActionIT.java index 57f231fa01340..bd6b7cc4df195 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/health/GetHealthActionIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/health/GetHealthActionIT.java @@ -19,6 +19,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.env.NodeEnvironment; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.plugins.HealthPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.repositories.RepositoriesService; @@ -138,7 +139,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { var status = clusterService.getClusterSettings().get(statusSetting); return createIndicator( status, diff --git a/server/src/internalClusterTest/java/org/elasticsearch/health/HealthServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/health/HealthServiceIT.java new file mode 100644 index 0000000000000..795854b007e58 --- /dev/null +++ b/server/src/internalClusterTest/java/org/elasticsearch/health/HealthServiceIT.java @@ -0,0 +1,169 @@ +/* + * 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.health; + +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.routing.allocation.decider.AllocationDeciders; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.TimeValue; +import org.elasticsearch.env.Environment; +import org.elasticsearch.env.NodeEnvironment; +import org.elasticsearch.health.node.DiskHealthInfo; +import org.elasticsearch.health.node.FetchHealthInfoCacheAction; +import org.elasticsearch.health.node.HealthInfo; +import org.elasticsearch.plugins.HealthPlugin; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.repositories.RepositoriesService; +import org.elasticsearch.script.ScriptService; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.InternalTestCluster; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.tracing.Tracer; +import org.elasticsearch.watcher.ResourceWatcherService; +import org.elasticsearch.xcontent.NamedXContentRegistry; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; + +import static org.elasticsearch.common.util.CollectionUtils.appendToCopy; +import static org.hamcrest.Matchers.equalTo; + +@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST) +public class HealthServiceIT extends ESIntegTestCase { + + @Override + protected Collection> nodePlugins() { + return appendToCopy(super.nodePlugins(), TestHealthPlugin.class); + } + + @Override + protected Settings nodeSettings(int ordinal, Settings otherSettings) { + /* + * Every once in a while a node tries to publish its health data before it has discovered the health node and gets a + * NodeNotConnectedException in LocalHealthMonitor. So it waits "health.reporting.local.monitor.interval" and tries again, this + * time successfully. Lowering that amount of time to the lowest allowed so that this test doesn't take any more time than + * necessary when that happens. + */ + return Settings.builder() + .put(super.nodeSettings(ordinal, otherSettings)) + .put("health.reporting.local.monitor.interval", TimeValue.timeValueSeconds(10)) + .build(); + } + + public void testThatHealthNodeDataIsFetchedAndPassedToIndicators() throws Exception { + try (InternalTestCluster internalCluster = internalCluster()) { + ensureStableCluster(internalCluster.getNodeNames().length); + waitForAllNodesToReportHealth(); + for (String node : internalCluster.getNodeNames()) { + HealthService healthService = internalCluster.getInstance(HealthService.class, node); + AtomicBoolean onResponseCalled = new AtomicBoolean(false); + ActionListener> listener = new ActionListener<>() { + @Override + public void onResponse(List resultList) { + /* + * The following is really just asserting that the TestHealthIndicatorService's calculate method was called. The + * assertions that it actually got the HealthInfo data are in the calculate method of TestHealthIndicatorService. + */ + assertNotNull(resultList); + assertThat(resultList.size(), equalTo(1)); + HealthIndicatorResult testIndicatorResult = resultList.get(0); + assertThat(testIndicatorResult.status(), equalTo(HealthStatus.RED)); + assertThat(testIndicatorResult.symptom(), equalTo(TestHealthIndicatorService.SYMPTOM)); + onResponseCalled.set(true); + } + + @Override + public void onFailure(Exception e) { + throw new RuntimeException(e); + } + }; + healthService.getHealth(internalCluster.client(node), TestHealthIndicatorService.NAME, true, listener); + assertBusy(() -> assertThat(onResponseCalled.get(), equalTo(true))); + } + } + } + + private void waitForAllNodesToReportHealth() throws Exception { + assertBusy(() -> { + ClusterState state = internalCluster().client() + .admin() + .cluster() + .prepareState() + .clear() + .setMetadata(true) + .setNodes(true) + .get() + .getState(); + FetchHealthInfoCacheAction.Response healthResponse = internalCluster().client() + .execute(FetchHealthInfoCacheAction.INSTANCE, new FetchHealthInfoCacheAction.Request()) + .get(); + for (String nodeId : state.getNodes().getNodes().keySet()) { + assertThat(healthResponse.getHealthInfo().diskInfoByNode().containsKey(nodeId), equalTo(true)); + } + }, 15, TimeUnit.SECONDS); + } + + public static final class TestHealthPlugin extends Plugin implements HealthPlugin { + + private final List healthIndicatorServices = new ArrayList<>(); + + @Override + public Collection createComponents( + Client client, + ClusterService clusterService, + ThreadPool threadPool, + ResourceWatcherService resourceWatcherService, + ScriptService scriptService, + NamedXContentRegistry xContentRegistry, + Environment environment, + NodeEnvironment nodeEnvironment, + NamedWriteableRegistry namedWriteableRegistry, + IndexNameExpressionResolver indexNameExpressionResolver, + Supplier repositoriesServiceSupplier, + Tracer tracer, + AllocationDeciders allocationDeciders + ) { + healthIndicatorServices.add(new TestHealthIndicatorService()); + return new ArrayList<>(healthIndicatorServices); + } + + @Override + public Collection getHealthIndicatorServices() { + return healthIndicatorServices; + } + } + + public static final class TestHealthIndicatorService implements HealthIndicatorService { + public static final String NAME = "test_indicator"; + public static final String SYMPTOM = "Symptom"; + + @Override + public String name() { + return NAME; + } + + @Override + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { + assertThat(healthInfo.diskInfoByNode().size(), equalTo(internalCluster().getNodeNames().length)); + for (DiskHealthInfo diskHealthInfo : healthInfo.diskInfoByNode().values()) { + assertThat(diskHealthInfo.healthStatus(), equalTo(HealthStatus.GREEN)); + } + return createIndicator(HealthStatus.RED, SYMPTOM, HealthIndicatorDetails.EMPTY, List.of(), List.of()); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java b/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java index aff14a5039d5b..4c0ad012e2932 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorService.java @@ -18,6 +18,7 @@ import org.elasticsearch.health.HealthIndicatorService; import org.elasticsearch.health.HealthStatus; import org.elasticsearch.health.ImpactArea; +import org.elasticsearch.health.node.HealthInfo; import java.util.Collection; import java.util.List; @@ -91,7 +92,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { CoordinationDiagnosticsService.CoordinationDiagnosticsResult coordinationDiagnosticsResult = coordinationDiagnosticsService .diagnoseMasterStability(explain); return getHealthIndicatorResult(coordinationDiagnosticsResult, explain); diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java index 94597db7d88f3..dbb2311e2e655 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorService.java @@ -42,6 +42,7 @@ import org.elasticsearch.health.HealthStatus; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.snapshots.SnapshotShardSizeInfo; import java.util.ArrayList; @@ -105,7 +106,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { var state = clusterService.state(); var shutdown = state.getMetadata().custom(NodesShutdownMetadata.TYPE, NodesShutdownMetadata.EMPTY); var status = new ShardAllocationStatus(state.getMetadata()); diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 8aac11e4326df..00b0744b6cf4b 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -68,6 +68,7 @@ import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.gateway.PersistedClusterStateService; import org.elasticsearch.health.node.LocalHealthMonitor; +import org.elasticsearch.health.node.action.TransportHealthNodeAction; import org.elasticsearch.health.node.selection.HealthNode; import org.elasticsearch.health.node.selection.HealthNodeTaskExecutor; import org.elasticsearch.http.HttpTransportSettings; @@ -526,7 +527,8 @@ public void apply(Settings value, Settings current, Settings previous) { MasterHistory.MAX_HISTORY_AGE_SETTING, ReadinessService.PORT, HealthNode.isEnabled() ? HealthNodeTaskExecutor.ENABLED_SETTING : null, - HealthNode.isEnabled() ? LocalHealthMonitor.POLL_INTERVAL_SETTING : null + HealthNode.isEnabled() ? LocalHealthMonitor.POLL_INTERVAL_SETTING : null, + HealthNode.isEnabled() ? TransportHealthNodeAction.HEALTH_NODE_TRANSPORT_ACTION_TIMEOUT : null ).filter(Objects::nonNull).collect(Collectors.toSet()); static List> BUILT_IN_SETTING_UPGRADERS = Collections.emptyList(); diff --git a/server/src/main/java/org/elasticsearch/health/GetHealthAction.java b/server/src/main/java/org/elasticsearch/health/GetHealthAction.java index c3bf5d0408842..d16fe3f10c3b9 100644 --- a/server/src/main/java/org/elasticsearch/health/GetHealthAction.java +++ b/server/src/main/java/org/elasticsearch/health/GetHealthAction.java @@ -14,6 +14,7 @@ import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.ActionType; import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; @@ -144,26 +145,34 @@ public static class TransportAction extends org.elasticsearch.action.support.Tra private final ClusterService clusterService; private final HealthService healthService; + private final NodeClient client; @Inject public TransportAction( ActionFilters actionFilters, TransportService transportService, ClusterService clusterService, - HealthService healthService + HealthService healthService, + NodeClient client ) { super(NAME, actionFilters, transportService.getTaskManager()); this.clusterService = clusterService; this.healthService = healthService; + this.client = client; } @Override - protected void doExecute(Task task, Request request, ActionListener listener) { - listener.onResponse( - new Response( - clusterService.getClusterName(), - healthService.getHealth(request.indicatorName, request.explain), - request.indicatorName == null + protected void doExecute(Task task, Request request, ActionListener responseListener) { + healthService.getHealth( + client, + request.indicatorName, + request.explain, + responseListener.map( + healthIndicatorResults -> new Response( + clusterService.getClusterName(), + healthIndicatorResults, + request.indicatorName == null + ) ) ); } diff --git a/server/src/main/java/org/elasticsearch/health/HealthIndicatorService.java b/server/src/main/java/org/elasticsearch/health/HealthIndicatorService.java index 4aab30fcd3205..264aad41fc331 100644 --- a/server/src/main/java/org/elasticsearch/health/HealthIndicatorService.java +++ b/server/src/main/java/org/elasticsearch/health/HealthIndicatorService.java @@ -8,6 +8,8 @@ package org.elasticsearch.health; +import org.elasticsearch.health.node.HealthInfo; + import java.util.Collection; import java.util.Comparator; import java.util.List; @@ -20,7 +22,7 @@ public interface HealthIndicatorService { String name(); - HealthIndicatorResult calculate(boolean explain); + HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo); /** * This method creates a HealthIndicatorResult with the given information. Note that it sorts the impacts by severity (the lower the diff --git a/server/src/main/java/org/elasticsearch/health/HealthService.java b/server/src/main/java/org/elasticsearch/health/HealthService.java index fde3c27012965..1051b6dead887 100644 --- a/server/src/main/java/org/elasticsearch/health/HealthService.java +++ b/server/src/main/java/org/elasticsearch/health/HealthService.java @@ -8,8 +8,15 @@ package org.elasticsearch.health; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.ResourceNotFoundException; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.client.internal.Client; import org.elasticsearch.core.Nullable; +import org.elasticsearch.health.node.FetchHealthInfoCacheAction; +import org.elasticsearch.health.node.HealthInfo; +import org.elasticsearch.health.node.selection.HealthNode; import java.util.Collections; import java.util.HashSet; @@ -36,6 +43,7 @@ public class HealthService { * Detail map key that contains the reasons a result was marked as UNKNOWN */ private static final String REASON = "reasons"; + private static final Logger logger = LogManager.getLogger(HealthService.class); private final List preflightHealthIndicatorServices; private final List healthIndicatorServices; @@ -61,15 +69,23 @@ public HealthService( /** * Returns the list of HealthIndicatorResult for this cluster. + * + * @param client A client to be used to fetch the health data from the health node * @param indicatorName If not null, the returned results will only have this indicator - * @param explain Whether to compute the details portion of the results - * @return A list of all HealthIndicatorResult if indicatorName is null, or one HealthIndicatorResult if indicatorName is not null + * @param explain Whether to compute the details portion of the results + * @param listener A listener to be notified of the list of all HealthIndicatorResult if indicatorName is null, or one + * HealthIndicatorResult if indicatorName is not null * @throws ResourceNotFoundException if an indicator name is given and the indicator is not found */ - public List getHealth(@Nullable String indicatorName, boolean explain) { + public void getHealth( + Client client, + @Nullable String indicatorName, + boolean explain, + ActionListener> listener + ) { // Determine if cluster is stable enough to calculate health before running other indicators List preflightResults = preflightHealthIndicatorServices.stream() - .map(service -> service.calculate(explain)) + .map(service -> service.calculate(explain, HealthInfo.EMPTY_HEALTH_INFO)) .toList(); // If any of these are not GREEN, then we cannot obtain health from other indicators @@ -79,31 +95,84 @@ public List getHealth(@Nullable String indicatorName, boo // Filter remaining indicators by indicator name if present before calculating their results Stream filteredIndicators = healthIndicatorServices.stream() .filter(service -> indicatorName == null || service.name().equals(indicatorName)); + Stream filteredPreflightResults = preflightResults.stream() + .filter(result -> indicatorName == null || result.name().equals(indicatorName)); - Stream filteredIndicatorResults; if (clusterHealthIsObtainable) { - // Calculate remaining indicators - filteredIndicatorResults = filteredIndicators.map(service -> service.calculate(explain)); + if (HealthNode.isEnabled()) { + client.execute(FetchHealthInfoCacheAction.INSTANCE, new FetchHealthInfoCacheAction.Request(), new ActionListener<>() { + @Override + public void onResponse(FetchHealthInfoCacheAction.Response response) { + HealthInfo healthInfo = response.getHealthInfo(); + validateResultsAndNotifyListener( + indicatorName, + Stream.concat( + filteredPreflightResults, + filteredIndicators.map(service -> service.calculate(explain, healthInfo)) + ).toList(), + listener + ); + } + + @Override + public void onFailure(Exception e) { + validateResultsAndNotifyListener( + indicatorName, + Stream.concat( + filteredPreflightResults, + filteredIndicators.map(service -> service.calculate(explain, HealthInfo.EMPTY_HEALTH_INFO)) + ).toList(), + listener + ); + } + }); + } else { + validateResultsAndNotifyListener( + indicatorName, + Stream.concat( + filteredPreflightResults, + filteredIndicators.map(service -> service.calculate(explain, HealthInfo.EMPTY_HEALTH_INFO)) + ).toList(), + listener + ); + } } else { // Mark remaining indicators as UNKNOWN HealthIndicatorDetails unknownDetails = healthUnknownReason(preflightResults, explain); - filteredIndicatorResults = filteredIndicators.map( + Stream filteredIndicatorResults = filteredIndicators.map( service -> generateUnknownResult(service, UNKNOWN_RESULT_SUMMARY_PREFLIGHT_FAILED, unknownDetails) ); + validateResultsAndNotifyListener( + indicatorName, + Stream.concat(filteredPreflightResults, filteredIndicatorResults).toList(), + listener + ); } + } - // Filter the cluster indicator results by indicator name if present - Stream filteredPreflightResults = preflightResults.stream() - .filter(result -> indicatorName == null || result.name().equals(indicatorName)); - - List results = Stream.concat(filteredPreflightResults, filteredIndicatorResults).toList(); + /** + * This method validates the health indicator results, and notifies the listener. If assertions are enabled and there are indicators + * with duplicate names, an AssertionError is thrown (the listener is not notified). If there are no results and the indicator name is + * not null, the listener will be notified of failure because the user could not get the results that were asked for. Otherwise, the + * listener will be notified with the results. + * + * @param indicatorName If not null, the results will be validated to only have this indicator name + * @param results The results that the listener will be notified of, if they pass validation + * @param listener A listener to be notified of results + */ + private void validateResultsAndNotifyListener( + @Nullable String indicatorName, + List results, + ActionListener> listener + ) { assert findDuplicatesByName(results).isEmpty() : String.format(Locale.ROOT, "Found multiple indicators with the same name: %s", findDuplicatesByName(results)); if (results.isEmpty() && indicatorName != null) { String errorMessage = String.format(Locale.ROOT, "Did not find indicator %s", indicatorName); - throw new ResourceNotFoundException(errorMessage); + listener.onFailure(new ResourceNotFoundException(errorMessage)); + } else { + listener.onResponse(results); } - return results; } /** diff --git a/server/src/main/java/org/elasticsearch/health/node/HealthInfo.java b/server/src/main/java/org/elasticsearch/health/node/HealthInfo.java index 3e3e0ccf9cbb3..d87609a904157 100644 --- a/server/src/main/java/org/elasticsearch/health/node/HealthInfo.java +++ b/server/src/main/java/org/elasticsearch/health/node/HealthInfo.java @@ -20,6 +20,8 @@ * @param diskInfoByNode A Map of node id to DiskHealthInfo for that node */ public record HealthInfo(Map diskInfoByNode) implements Writeable { + public static final HealthInfo EMPTY_HEALTH_INFO = new HealthInfo(Map.of()); + public HealthInfo(StreamInput input) throws IOException { this(input.readMap(StreamInput::readString, DiskHealthInfo::new)); } diff --git a/server/src/main/java/org/elasticsearch/health/node/action/TransportHealthNodeAction.java b/server/src/main/java/org/elasticsearch/health/node/action/TransportHealthNodeAction.java index a5b83fe94c794..72c43beeeadd0 100644 --- a/server/src/main/java/org/elasticsearch/health/node/action/TransportHealthNodeAction.java +++ b/server/src/main/java/org/elasticsearch/health/node/action/TransportHealthNodeAction.java @@ -19,6 +19,8 @@ import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.settings.Setting; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.health.node.selection.HealthNode; import org.elasticsearch.tasks.CancellableTask; import org.elasticsearch.tasks.Task; @@ -38,10 +40,23 @@ public abstract class TransportHealthNodeAction HEALTH_NODE_TRANSPORT_ACTION_TIMEOUT = Setting.timeSetting( + "health_node.transport_action_timeout", + TimeValue.timeValueSeconds(5), + TimeValue.timeValueMillis(1), + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); + protected final TransportService transportService; protected final ClusterService clusterService; protected final ThreadPool threadPool; protected final String executor; + private TimeValue healthNodeTransportActionTimeout; private final Writeable.Reader responseReader; @@ -61,6 +76,12 @@ protected TransportHealthNodeAction( this.threadPool = threadPool; this.executor = executor; this.responseReader = response; + this.healthNodeTransportActionTimeout = HEALTH_NODE_TRANSPORT_ACTION_TIMEOUT.get(clusterService.getSettings()); + clusterService.getClusterSettings() + .addSettingsUpdateConsumer( + HEALTH_NODE_TRANSPORT_ACTION_TIMEOUT, + newTimeout -> this.healthNodeTransportActionTimeout = newTimeout + ); } protected abstract void healthOperation(Task task, Request request, ClusterState state, ActionListener listener) @@ -105,7 +126,14 @@ public void handleException(final TransportException exception) { } }; if (task != null) { - transportService.sendChildRequest(healthNode, actionName, request, task, TransportRequestOptions.EMPTY, handler); + transportService.sendChildRequest( + healthNode, + actionName, + request, + task, + TransportRequestOptions.timeout(healthNodeTransportActionTimeout), + handler + ); } else { transportService.sendRequest(healthNode, actionName, request, handler); } diff --git a/server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java b/server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java index e10bd2969ac30..c954ff94f31fe 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorService.java @@ -18,6 +18,7 @@ import org.elasticsearch.health.HealthIndicatorService; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.repositories.RepositoryData; import java.util.Collections; @@ -66,7 +67,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { var snapshotMetadata = clusterService.state().metadata().custom(RepositoriesMetadata.TYPE, RepositoriesMetadata.EMPTY); if (snapshotMetadata.repositories().isEmpty()) { diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java index 48300520fcd92..2790e846bf018 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/StableMasterHealthIndicatorServiceTests.java @@ -25,6 +25,7 @@ import org.elasticsearch.health.HealthIndicatorDetails; import org.elasticsearch.health.HealthIndicatorResult; import org.elasticsearch.health.HealthStatus; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xcontent.ToXContent; @@ -245,7 +246,7 @@ public void testCalculate() throws Exception { // Change 4: localMasterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node2MasterClusterState, node3MasterClusterState)); - HealthIndicatorResult result = service.calculate(true); + HealthIndicatorResult result = service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO); assertThat(result.status(), equalTo(HealthStatus.YELLOW)); assertThat(result.symptom(), equalTo("The elected master node has changed 4 times in the last 30m")); assertThat(result.impacts().size(), equalTo(3)); diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java index 0d063138c7d84..611b827a5eb38 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ShardsAvailabilityHealthIndicatorServiceTests.java @@ -40,6 +40,7 @@ import org.elasticsearch.health.HealthStatus; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.index.Index; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.test.ESTestCase; @@ -106,7 +107,7 @@ public void testShouldBeGreenWhenAllPrimariesAndReplicasAreStarted() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( GREEN, @@ -136,7 +137,7 @@ public void testShouldBeYellowWhenThereAreUnassignedReplicas() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( YELLOW, @@ -172,7 +173,7 @@ public void testShouldBeRedWhenThereAreUnassignedPrimariesAndAssignedReplicas() var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( RED, @@ -196,7 +197,7 @@ public void testShouldBeRedWhenThereAreUnassignedPrimariesAndNoReplicas() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( RED, @@ -222,7 +223,7 @@ public void testShouldBeRedWhenThereAreUnassignedPrimariesAndUnassignedReplicasO ); var service = createAllocationHealthIndicatorService(clusterState); - HealthIndicatorResult result = service.calculate(true); + HealthIndicatorResult result = service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO); assertEquals(RED, result.status()); assertEquals("This cluster has 1 unavailable primary, 1 unavailable replica.", result.symptom()); assertEquals(1, result.impacts().size()); @@ -252,7 +253,7 @@ public void testShouldBeRedWhenThereAreUnassignedPrimariesAndUnassignedReplicasO ); var service = createAllocationHealthIndicatorService(clusterState); - HealthIndicatorResult result = service.calculate(true); + HealthIndicatorResult result = service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO); assertEquals(RED, result.status()); assertEquals("This cluster has 1 unavailable primary, 2 unavailable replicas.", result.symptom()); assertEquals(2, result.impacts().size()); @@ -296,7 +297,7 @@ public void testSortByIndexPriority() { ); var service = createAllocationHealthIndicatorService(clusterState); - HealthIndicatorResult result = service.calculate(true); + HealthIndicatorResult result = service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO); // index-2 has the higher priority so it ought to be listed first, followed by index-1 then index-3 which have the same priority: assertThat( result.impacts().get(0), @@ -325,7 +326,7 @@ public void testShouldBeGreenWhenThereAreRestartingReplicas() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( GREEN, @@ -346,7 +347,7 @@ public void testShouldBeGreenWhenThereAreNoReplicasExpected() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( GREEN, @@ -373,7 +374,7 @@ public void testShouldBeYellowWhenRestartingReplicasReachedAllocationDelay() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( YELLOW, @@ -401,7 +402,7 @@ public void testShouldBeGreenWhenThereAreInitializingPrimaries() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( GREEN, @@ -422,7 +423,7 @@ public void testShouldBeGreenWhenThereAreRestartingPrimaries() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( GREEN, @@ -448,7 +449,7 @@ public void testShouldBeRedWhenRestartingPrimariesReachedAllocationDelayAndNoRep var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedResult( RED, @@ -486,7 +487,7 @@ public void testUserActionsNotGeneratedWhenNotDrillingDown() { var service = createAllocationHealthIndicatorService(clusterState); assertThat( - service.calculate(false), + service.calculate(false, HealthInfo.EMPTY_HEALTH_INFO), equalTo( createExpectedTruncatedResult( RED, diff --git a/server/src/test/java/org/elasticsearch/health/HealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/health/HealthIndicatorServiceTests.java index 763b081e834b8..20bbcc2b20f76 100644 --- a/server/src/test/java/org/elasticsearch/health/HealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/health/HealthIndicatorServiceTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.health; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.test.ESTestCase; import java.util.Collections; @@ -59,7 +60,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { return null; } }; diff --git a/server/src/test/java/org/elasticsearch/health/HealthServiceTests.java b/server/src/test/java/org/elasticsearch/health/HealthServiceTests.java index e7a00f8ecefd3..ef1c0e945dd93 100644 --- a/server/src/test/java/org/elasticsearch/health/HealthServiceTests.java +++ b/server/src/test/java/org/elasticsearch/health/HealthServiceTests.java @@ -9,25 +9,37 @@ package org.elasticsearch.health; import org.elasticsearch.ResourceNotFoundException; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionType; +import org.elasticsearch.client.internal.node.NodeClient; +import org.elasticsearch.health.node.DiskHealthInfo; +import org.elasticsearch.health.node.FetchHealthInfoCacheAction; +import org.elasticsearch.health.node.HealthInfo; +import org.elasticsearch.health.node.selection.HealthNode; import org.elasticsearch.test.ESTestCase; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Stream; import static org.elasticsearch.health.HealthStatus.GREEN; import static org.elasticsearch.health.HealthStatus.RED; import static org.elasticsearch.health.HealthStatus.UNKNOWN; import static org.elasticsearch.health.HealthStatus.YELLOW; -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.is; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; public class HealthServiceTests extends ESTestCase { - public void testShouldReturnGroupedIndicators() { + public void testShouldReturnGroupedIndicators() throws Exception { var networkLatency = new HealthIndicatorResult("network_latency", GREEN, null, null, null, null); var slowTasks = new HealthIndicatorResult("slow_task_assignment", YELLOW, null, null, null, null); @@ -42,14 +54,48 @@ public void testShouldReturnGroupedIndicators() { ) ); - assertThat(service.getHealth(null, false), hasItems(slowTasks, networkLatency, shardsAvailable)); + NodeClient client = getTestClient(HealthInfo.EMPTY_HEALTH_INFO); - assertThat(service.getHealth(null, false), hasItems(slowTasks, networkLatency, networkLatency, slowTasks)); + assertExpectedHealthIndicatorResults(service, client, null, slowTasks, networkLatency, shardsAvailable); + assertExpectedHealthIndicatorResults(service, client, "slow_task_assignment", slowTasks); + } + + private void assertExpectedHealthIndicatorResults( + HealthService service, + NodeClient client, + String indicatorName, + HealthIndicatorResult... expectedHealthIndicatorResults + ) throws Exception { + AtomicBoolean onResponseCalled = new AtomicBoolean(false); + service.getHealth( + client, + indicatorName, + false, + getExpectedHealthIndicatorResultsActionListener(onResponseCalled, expectedHealthIndicatorResults) + ); + assertBusy(() -> assertThat(onResponseCalled.get(), equalTo(true))); + } + + private ActionListener> getExpectedHealthIndicatorResultsActionListener( + AtomicBoolean onResponseCalled, + HealthIndicatorResult... healthIndicatorResults + ) { + return new ActionListener<>() { + @Override + public void onResponse(List results) { + assertThat(results.size(), equalTo(healthIndicatorResults.length)); + assertThat(results, hasItems(healthIndicatorResults)); + onResponseCalled.set(true); + } - assertThat(service.getHealth("slow_task_assignment", false), hasItems(slowTasks)); + @Override + public void onFailure(Exception e) { + throw new RuntimeException(e); + } + }; } - public void testDuplicateIndicatorNames() { + public void testDuplicateIndicatorNames() throws Exception { // Same indicator name, should throw exception: var networkLatency = new HealthIndicatorResult( "network_latency", @@ -68,10 +114,12 @@ public void testDuplicateIndicatorNames() { createMockHealthIndicatorService(networkLatency) ) ); - expectThrows(AssertionError.class, () -> service.getHealth(null, true)); + NodeClient client = getTestClient(HealthInfo.EMPTY_HEALTH_INFO); + // This is testing an assertion, so we expect it to blow up in place rather than calling onFailure: + assertGetHealthThrowsException(service, client, null, true, AssertionError.class, null, false); } - public void testMissingIndicator() { + public void testMissingIndicator() throws Exception { var networkLatency = new HealthIndicatorResult("network_latency", GREEN, null, null, null, null); var slowTasks = new HealthIndicatorResult("slow_task_assignment", YELLOW, null, null, null, null); var shardsAvailable = new HealthIndicatorResult("shards_availability", GREEN, null, null, null, null); @@ -84,11 +132,78 @@ public void testMissingIndicator() { createMockHealthIndicatorService(shardsAvailable) ) ); + NodeClient client = getTestClient(HealthInfo.EMPTY_HEALTH_INFO); + assertGetHealthThrowsException( + service, + client, + "indicator99", + false, + ResourceNotFoundException.class, + "Did not find indicator indicator99", + true + ); + } - expectThrows(ResourceNotFoundException.class, "Did not find indicator indicator99", () -> service.getHealth("indicator99", false)); + private void assertGetHealthThrowsException( + HealthService service, + NodeClient client, + String indicatorName, + boolean explain, + Class expectedType, + String expectedMessage, + boolean expectOnFailCalled + ) throws Exception { + AtomicBoolean onFailureCalled = new AtomicBoolean(false); + ActionListener> listener = getExpectThrowsActionListener( + onFailureCalled, + expectedType, + expectedMessage + ); + try { + service.getHealth(client, indicatorName, explain, listener); + } catch (Throwable t) { + if (expectOnFailCalled || (expectedType.isInstance(t) == false)) { + throw new RuntimeException("Unexpected throwable", t); + } else { + // Expected + if (expectedMessage != null) { + assertThat(t.getMessage(), equalTo(expectedMessage)); + } + } + } + if (expectOnFailCalled) { + assertBusy(() -> assertThat(onFailureCalled.get(), equalTo(true))); + } } - public void testPreflightIndicatorResultsPresent() { + private ActionListener> getExpectThrowsActionListener( + AtomicBoolean onFailureCalled, + Class expectedType, + String expectedMessage + ) { + return new ActionListener<>() { + @Override + public void onResponse(List healthIndicatorResults) { + fail("Expected failure"); + } + + @Override + public void onFailure(Exception e) { + if (expectedType.isInstance(e)) { + if (expectedMessage == null) { + onFailureCalled.set(true); + } else { + assertThat(e.getMessage(), equalTo(expectedMessage)); + onFailureCalled.set(true); + } + } else { + throw new RuntimeException("Unexpected exception", e); + } + } + }; + } + + public void testPreflightIndicatorResultsPresent() throws Exception { // Preflight check var hasMaster = new HealthIndicatorResult("has_master", GREEN, null, null, null, null); // Other indicators @@ -104,21 +219,49 @@ public void testPreflightIndicatorResultsPresent() { createMockHealthIndicatorService(shardsAvailable) ) ); + NodeClient client = getTestClient(HealthInfo.EMPTY_HEALTH_INFO); // Get all indicators returns preflight result mixed in with the other indicators - List health = service.getHealth(null, false); - assertThat(health.size(), is(equalTo(4))); - assertThat(health, containsInAnyOrder(hasMaster, networkLatency, slowTasks, shardsAvailable)); + assertExpectedHealthIndicatorResults(service, client, null, hasMaster, networkLatency, slowTasks, shardsAvailable); // Getting single indicator returns correct indicator still - health = service.getHealth("slow_task_assignment", false); - assertThat(health.size(), is(equalTo(1))); - assertThat(health, contains(slowTasks)); + assertExpectedHealthIndicatorResults(service, client, "slow_task_assignment", slowTasks); // Getting single preflight indicator returns preflight indicator correctly - health = service.getHealth("has_master", false); - assertThat(health.size(), is(equalTo(1))); - assertThat(health, contains(hasMaster)); + assertExpectedHealthIndicatorResults(service, client, "has_master", hasMaster); + } + + public void testThatIndicatorsGetHealthInfoData() throws Exception { + /* + * This test makes sure that HealthService is passing the data returned by the FetchHealthInfoCacheAction to all of the + * HealthIndicatorServices except for the preflight ones. + */ + // Preflight check + var hasMaster = new HealthIndicatorResult("has_master", GREEN, null, null, null, null); + // Other indicators + var networkLatency = new HealthIndicatorResult("network_latency", GREEN, null, null, null, null); + var slowTasks = new HealthIndicatorResult("slow_task_assignment", YELLOW, null, null, null, null); + var shardsAvailable = new HealthIndicatorResult("shards_availability", GREEN, null, null, null, null); + Map diskHealthInfoMap = new HashMap<>(); + diskHealthInfoMap.put( + randomAlphaOfLength(30), + new DiskHealthInfo(randomFrom(HealthStatus.values()), randomFrom(DiskHealthInfo.Cause.values())) + ); + HealthInfo healthInfo = new HealthInfo(diskHealthInfoMap); + + var service = new HealthService( + // The preflight indicator does not get data because the data is not fetched until after the preflight check + List.of(createMockHealthIndicatorService(hasMaster, HealthInfo.EMPTY_HEALTH_INFO)), + List.of( + createMockHealthIndicatorService(networkLatency, healthInfo), + createMockHealthIndicatorService(slowTasks, healthInfo), + createMockHealthIndicatorService(shardsAvailable, healthInfo) + ) + ); + NodeClient client = getTestClient(healthInfo); + + // Get all indicators returns preflight result mixed in with the other indicators + assertExpectedHealthIndicatorResults(service, client, null, hasMaster, networkLatency, slowTasks, shardsAvailable); } private void assertIndicatorIsUnknownStatus(HealthIndicatorResult result) { @@ -126,7 +269,7 @@ private void assertIndicatorIsUnknownStatus(HealthIndicatorResult result) { assertThat(result.symptom(), is(HealthService.UNKNOWN_RESULT_SUMMARY_PREFLIGHT_FAILED)); } - public void testPreflightIndicatorFailureTriggersUnknownResults() { + public void testPreflightIndicatorFailureTriggersUnknownResults() throws Exception { // Preflight checks var hasMaster = new HealthIndicatorResult("has_master", RED, null, null, null, null); var hasStorage = new HealthIndicatorResult("has_storage", GREEN, null, null, null, null); @@ -143,9 +286,9 @@ public void testPreflightIndicatorFailureTriggersUnknownResults() { createMockHealthIndicatorService(shardsAvailable) ) ); - + NodeClient client = getTestClient(HealthInfo.EMPTY_HEALTH_INFO); { - List health = service.getHealth(null, false); + List health = getHealthIndicatorResults(service, client, null); assertThat(health.size(), is(equalTo(5))); // Preflight indicators unchanged; posflight all say List nonPreflightNames = Stream.of(networkLatency, slowTasks, shardsAvailable) @@ -159,19 +302,66 @@ public void testPreflightIndicatorFailureTriggersUnknownResults() { } { - List health = service.getHealth("slow_task_assignment", false); + List health = getHealthIndicatorResults(service, client, "slow_task_assignment"); assertThat(health.size(), is(equalTo(1))); assertIndicatorIsUnknownStatus(health.get(0)); } { - List health = service.getHealth("has_master", false); + List health = getHealthIndicatorResults(service, client, "has_master"); assertThat(health.size(), is(equalTo(1))); assertThat(health.get(0), is(equalTo(hasMaster))); } } + private List getHealthIndicatorResults(HealthService service, NodeClient client, String indicatorName) + throws Exception { + AtomicReference> resultReference = new AtomicReference<>(); + ActionListener> listener = new ActionListener<>() { + @Override + public void onResponse(List healthIndicatorResults) { + resultReference.set(healthIndicatorResults); + } + + @Override + public void onFailure(Exception e) { + throw new RuntimeException(e); + } + }; + service.getHealth(client, indicatorName, false, listener); + assertBusy(() -> assertNotNull(resultReference.get())); + return resultReference.get(); + } + + /** + * This returns a mocked NodeClient that will return the given HealthInfo if the FetchHealthInfoCacheAction is called. + * @param healthInfo The HealthInfo that will be returned if this client calls the FetchHealthInfoCacheAction + * @return A mocked NodeClient + */ + @SuppressWarnings("unchecked") + private NodeClient getTestClient(HealthInfo healthInfo) { + NodeClient client = mock(NodeClient.class); + doAnswer(invocation -> { + ActionListener actionListener = invocation.getArgument(2, ActionListener.class); + actionListener.onResponse(new FetchHealthInfoCacheAction.Response(healthInfo)); + return null; + }).when(client).doExecute(any(ActionType.class), any(), any(ActionListener.class)); + return client; + } + private static HealthIndicatorService createMockHealthIndicatorService(HealthIndicatorResult result) { + return createMockHealthIndicatorService(result, null); + } + + /** + * This returns a test HealthIndicatorService + * @param result The HealthIndicatorResult that will be returned by the calculate method when the HealthIndicatorService returned by + * this method is called + * @param expectedHealthInfo If this HealthInfo is not null then the returned HealthIndicatorService's calculate method will assert + * that the HealthInfo it is passed is equal to this when it is called + * @return A test HealthIndicatorService + */ + private static HealthIndicatorService createMockHealthIndicatorService(HealthIndicatorResult result, HealthInfo expectedHealthInfo) { return new HealthIndicatorService() { @Override public String name() { @@ -179,10 +369,12 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { + if (expectedHealthInfo != null && HealthNode.isEnabled()) { + assertThat(healthInfo, equalTo(expectedHealthInfo)); + } return result; } }; } - } diff --git a/server/src/test/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceTests.java b/server/src/test/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceTests.java index 514b2a4cb3bfc..a18825fadc662 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/RepositoryIntegrityHealthIndicatorServiceTests.java @@ -21,6 +21,7 @@ import org.elasticsearch.health.HealthIndicatorResult; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.test.ESTestCase; import java.util.Collections; @@ -46,7 +47,7 @@ public void testIsGreenWhenAllRepositoriesAreNotCorrupted() { var service = createRepositoryCorruptionHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -70,7 +71,7 @@ public void testIsRedWhenAtLeastOneRepoIsCorrupted() { List corruptedRepos = List.of("corrupted-repo"); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -97,7 +98,7 @@ public void testIsGreenWhenNoMetadata() { var service = createRepositoryCorruptionHealthIndicatorService(clusterState); assertThat( - service.calculate(false), + service.calculate(false, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorService.java index b590e40780762..a313c9a7995c7 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorService.java @@ -15,6 +15,7 @@ import org.elasticsearch.health.HealthIndicatorService; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata; import org.elasticsearch.xpack.core.ilm.OperationMode; @@ -60,7 +61,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { var ilmMetadata = clusterService.state().metadata().custom(IndexLifecycleMetadata.TYPE, IndexLifecycleMetadata.EMPTY); if (ilmMetadata.getPolicyMetadatas().isEmpty()) { return createIndicator( diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorService.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorService.java index 4d44d580aa830..a5609c73d5fc3 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorService.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorService.java @@ -16,6 +16,7 @@ import org.elasticsearch.health.HealthIndicatorService; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.xpack.core.ilm.OperationMode; import org.elasticsearch.xpack.core.slm.SnapshotInvocationRecord; import org.elasticsearch.xpack.core.slm.SnapshotLifecycleMetadata; @@ -91,7 +92,7 @@ public String name() { } @Override - public HealthIndicatorResult calculate(boolean explain) { + public HealthIndicatorResult calculate(boolean explain, HealthInfo healthInfo) { var slmMetadata = clusterService.state().metadata().custom(SnapshotLifecycleMetadata.TYPE, SnapshotLifecycleMetadata.EMPTY); if (slmMetadata.getSnapshotConfigurations().isEmpty()) { return createIndicator( diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorServiceTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorServiceTests.java index c4e2d19d722f5..4cd65f7c4d4dd 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorServiceTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/IlmHealthIndicatorServiceTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.health.HealthIndicatorResult; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ilm.IndexLifecycleMetadata; import org.elasticsearch.xpack.core.ilm.LifecyclePolicy; @@ -41,7 +42,7 @@ public void testIsGreenWhenRunningAndPoliciesConfigured() { var service = createIlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -61,7 +62,7 @@ public void testIsYellowWhenNotRunningAndPoliciesConfigured() { var service = createIlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -88,7 +89,7 @@ public void testIsGreenWhenNotRunningAndNoPolicies() { var service = createIlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -107,7 +108,7 @@ public void testIsGreenWhenNoMetadata() { var service = createIlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorServiceTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorServiceTests.java index 5507620c82228..ad7cf1246ec2b 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorServiceTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SlmHealthIndicatorServiceTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.health.HealthIndicatorResult; import org.elasticsearch.health.ImpactArea; import org.elasticsearch.health.SimpleHealthIndicatorDetails; +import org.elasticsearch.health.node.HealthInfo; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ilm.LifecycleSettings; import org.elasticsearch.xpack.core.slm.SnapshotInvocationRecord; @@ -55,7 +56,7 @@ public void testIsGreenWhenRunningAndPoliciesConfigured() { var service = createSlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -75,7 +76,7 @@ public void testIsYellowWhenNotRunningAndPoliciesConfigured() { var service = createSlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -101,7 +102,7 @@ public void testIsGreenWhenNotRunningAndNoPolicies() { var service = createSlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -120,7 +121,7 @@ public void testIsGreenWhenNoMetadata() { var service = createSlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -151,7 +152,7 @@ public void testIsGreenWhenPoliciesHaveFailedForLessThanWarningThreshold() { var service = createSlmHealthIndicatorService(clusterState); assertThat( - service.calculate(true), + service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO), equalTo( new HealthIndicatorResult( NAME, @@ -182,7 +183,7 @@ public void testIsYellowWhenPoliciesHaveFailedForMoreThanWarningThreshold() { ); var service = createSlmHealthIndicatorService(clusterState); - HealthIndicatorResult calculate = service.calculate(true); + HealthIndicatorResult calculate = service.calculate(true, HealthInfo.EMPTY_HEALTH_INFO); assertThat( calculate, equalTo( From 514e91424ec2496a13a9ffa409d297151f7fa079 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Wed, 14 Sep 2022 16:13:56 -0400 Subject: [PATCH 11/12] [TEST] Add mock doublevalues script (#90073) --- .../script/DoubleValuesScriptTests.java | 38 ++++++++++++++++ .../script/MockScriptEngine.java | 44 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 server/src/test/java/org/elasticsearch/script/DoubleValuesScriptTests.java diff --git a/server/src/test/java/org/elasticsearch/script/DoubleValuesScriptTests.java b/server/src/test/java/org/elasticsearch/script/DoubleValuesScriptTests.java new file mode 100644 index 0000000000000..8884de4aa9df9 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/script/DoubleValuesScriptTests.java @@ -0,0 +1,38 @@ +/* + * 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.script; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; + +public class DoubleValuesScriptTests extends ESTestCase { + private ScriptService buildScriptService() { + Map, Object>> scripts = new HashMap<>(); + for (int i = 0; i < 20; ++i) { + scripts.put(i + "+" + i, p -> null); // only care about compilation, not execution + } + var scriptEngine = new MockScriptEngine("test", scripts, Collections.emptyMap()); + return new ScriptService(Settings.EMPTY, Map.of("test", scriptEngine), new HashMap<>(ScriptModule.CORE_CONTEXTS), () -> 1L); + } + + public void testDoubleValuesScriptContextCanBeCompiled() throws IOException { + var scriptService = buildScriptService(); + Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); + var result = scriptService.compile(script, DoubleValuesScript.CONTEXT).newInstance(); + + assertEquals(1, result.execute(), 0.0001); + assertEquals(0, result.variables().length); + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java index 7996f3b36fde7..b1cc863835314 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -9,7 +9,11 @@ package org.elasticsearch.script; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.DoubleValues; +import org.apache.lucene.search.DoubleValuesSource; +import org.apache.lucene.search.Rescorer; import org.apache.lucene.search.Scorable; +import org.apache.lucene.search.SortField; import org.elasticsearch.common.util.Maps; import org.elasticsearch.index.query.IntervalFilterScript; import org.elasticsearch.index.similarity.ScriptedSimilarity.Doc; @@ -339,6 +343,9 @@ public void execute() { } }; return context.factoryClazz.cast(objectFieldScript); + } else if (context.instanceClazz.equals(DoubleValuesScript.class)) { + DoubleValuesScript.Factory doubleValuesScript = () -> new MockDoubleValuesScript(); + return context.factoryClazz.cast(doubleValuesScript); } ContextCompiler compiler = contexts.get(context); if (compiler != null) { @@ -856,4 +863,41 @@ public BytesRefProducer execute() { }; } } + + class MockDoubleValuesScript extends DoubleValuesScript { + @Override + public double execute() { + return 1.0; + } + + @Override + public double evaluate(DoubleValues[] functionValues) { + return 1.0; + } + + @Override + public DoubleValuesSource getDoubleValuesSource(Function sourceProvider) { + return null; + } + + @Override + public SortField getSortField(Function sourceProvider, boolean reverse) { + return null; + } + + @Override + public Rescorer getRescorer(Function sourceProvider) { + return null; + } + + @Override + public String sourceText() { + return null; + } + + @Override + public String[] variables() { + return new String[0]; + } + } } From f5d61fbe6ad923b2c80ea28c6257ce27f084f397 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 15 Sep 2022 10:56:01 +1000 Subject: [PATCH 12/12] [Doc] Fix typo for the default role mapping file (#90049) The file name should be role_mapping.yml instead of role_mappings.yml, i.e. NOT plural. --- x-pack/docs/en/security/authorization/mapping-roles.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/docs/en/security/authorization/mapping-roles.asciidoc b/x-pack/docs/en/security/authorization/mapping-roles.asciidoc index f9c1363e5c01c..d6499bd7e6784 100644 --- a/x-pack/docs/en/security/authorization/mapping-roles.asciidoc +++ b/x-pack/docs/en/security/authorization/mapping-roles.asciidoc @@ -86,7 +86,7 @@ this is a common setting in Elasticsearch, changing its value might effect other schedules in the system. While the _role mapping APIs_ is the preferred way to manage role mappings, using -the `role_mappings.yml` file becomes useful in a couple of use cases: +the `role_mapping.yml` file becomes useful in a couple of use cases: . If you want to define fixed role mappings that no one (besides an administrator with physical access to the {es} nodes) would be able to change. @@ -96,7 +96,7 @@ need to have their roles mapped to them even when the cluster is RED. For instan an administrator that authenticates via LDAP or PKI and gets assigned an administrator role so that they can perform corrective actions. -Please note however, that the role_mappings.yml file is provided +Please note however, that the `role_mapping.yml` file is provided as a minimal administrative function and is not intended to cover and be used to define roles for all use cases.