From ef4277c14c4855e4c1822d5e3104f7e09d03d5eb Mon Sep 17 00:00:00 2001 From: Nicholas Walter Knize Date: Tue, 1 Feb 2022 12:30:24 -0600 Subject: [PATCH 1/4] [Remove] Segment memory estimation and tracking Lucene 9 removed CodecReader#ramBytesUsed and all file formats that no longer consume large amounts of memory. As a result RAM estimation for segments is no longer possible and needs to be removed. backwards compatibility is retained for the tranport layer. Signed-off-by: Nicholas Walter Knize --- .../opensearch/index/shard/IndexShardIT.java | 89 --------- .../indices/stats/IndexStatsIT.java | 1 - .../segments/IndicesSegmentResponse.java | 2 +- .../admin/indices/stats/CommonStats.java | 5 +- .../org/opensearch/index/engine/Engine.java | 19 +- .../index/engine/InternalEngine.java | 5 +- .../index/engine/OpenSearchReaderManager.java | 14 +- .../engine/RamAccountingRefreshListener.java | 94 --------- .../index/engine/ReadOnlyEngine.java | 9 +- .../org/opensearch/index/engine/Segment.java | 25 ++- .../index/engine/SegmentsStats.java | 180 ++++-------------- .../rest/action/cat/RestIndicesAction.java | 4 +- .../rest/action/cat/RestNodesAction.java | 2 +- .../rest/action/cat/RestSegmentsAction.java | 2 +- .../rest/action/cat/RestShardsAction.java | 2 +- .../indices/stats/IndicesStatsTests.java | 31 +-- .../index/engine/InternalEngineTests.java | 2 - .../index/engine/NoOpEngineTests.java | 1 - .../opensearch/index/engine/SegmentTests.java | 2 - .../index/shard/IndexShardTests.java | 170 ----------------- .../opensearch/test/ExternalTestCluster.java | 7 +- 21 files changed, 78 insertions(+), 588 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/engine/RamAccountingRefreshListener.java diff --git a/server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java b/server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java index ac45940b9913c..28f5eb023b187 100644 --- a/server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java @@ -36,8 +36,6 @@ import org.opensearch.ExceptionsHelper; import org.opensearch.Version; import org.opensearch.action.ActionListener; -import org.opensearch.action.admin.cluster.node.stats.NodeStats; -import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchResponse; @@ -57,7 +55,6 @@ import org.opensearch.common.CheckedRunnable; import org.opensearch.common.Strings; import org.opensearch.common.UUIDs; -import org.opensearch.common.breaker.CircuitBreaker; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.lucene.uid.Versions; import org.opensearch.common.settings.Settings; @@ -76,7 +73,6 @@ import org.opensearch.index.engine.CommitStats; import org.opensearch.index.engine.Engine; import org.opensearch.index.engine.NoOpEngine; -import org.opensearch.index.engine.SegmentsStats; import org.opensearch.index.flush.FlushStats; import org.opensearch.index.mapper.SourceToParse; import org.opensearch.index.seqno.RetentionLeaseSyncer; @@ -86,10 +82,8 @@ import org.opensearch.index.translog.TranslogStats; import org.opensearch.indices.IndicesService; import org.opensearch.indices.breaker.CircuitBreakerService; -import org.opensearch.indices.breaker.CircuitBreakerStats; import org.opensearch.indices.recovery.RecoveryState; import org.opensearch.plugins.Plugin; -import org.opensearch.search.aggregations.AggregationBuilders; import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.test.DummyShardLock; import org.opensearch.test.OpenSearchSingleNodeTestCase; @@ -122,7 +116,6 @@ import static com.carrotsearch.randomizedtesting.RandomizedTest.randomAsciiLettersOfLength; import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; -import static org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest.Metric.BREAKER; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.opensearch.action.support.WriteRequest.RefreshPolicy.NONE; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; @@ -135,13 +128,11 @@ import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures; import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.lessThanOrEqualTo; -import static org.hamcrest.Matchers.notNullValue; public class IndexShardIT extends OpenSearchSingleNodeTestCase { @@ -643,86 +634,6 @@ public void postDelete(ShardId shardId, Engine.Delete delete, Engine.DeleteResul } } - /** Check that the accounting breaker correctly matches the segments API for memory usage */ - private void checkAccountingBreaker() { - CircuitBreakerService breakerService = getInstanceFromNode(CircuitBreakerService.class); - CircuitBreaker acctBreaker = breakerService.getBreaker(CircuitBreaker.ACCOUNTING); - long usedMem = acctBreaker.getUsed(); - assertThat(usedMem, greaterThan(0L)); - NodesStatsResponse response = client().admin().cluster().prepareNodesStats().setIndices(true).addMetric(BREAKER.metricName()).get(); - NodeStats stats = response.getNodes().get(0); - assertNotNull(stats); - SegmentsStats segmentsStats = stats.getIndices().getSegments(); - CircuitBreakerStats breakerStats = stats.getBreaker().getStats(CircuitBreaker.ACCOUNTING); - assertEquals(usedMem, segmentsStats.getMemoryInBytes()); - assertEquals(usedMem, breakerStats.getEstimated()); - } - - public void testCircuitBreakerIncrementedByIndexShard() throws Exception { - client().admin() - .cluster() - .prepareUpdateSettings() - .setTransientSettings(Settings.builder().put("network.breaker.inflight_requests.overhead", 0.0)) - .get(); - - // Generate a couple of segments - client().prepareIndex("test", "_doc", "1") - .setSource("{\"foo\":\"" + randomAlphaOfLength(100) + "\"}", XContentType.JSON) - .setRefreshPolicy(IMMEDIATE) - .get(); - // Use routing so 2 documents are guaranteed to be on the same shard - String routing = randomAlphaOfLength(5); - client().prepareIndex("test", "_doc", "2") - .setSource("{\"foo\":\"" + randomAlphaOfLength(100) + "\"}", XContentType.JSON) - .setRefreshPolicy(IMMEDIATE) - .setRouting(routing) - .get(); - client().prepareIndex("test", "_doc", "3") - .setSource("{\"foo\":\"" + randomAlphaOfLength(100) + "\"}", XContentType.JSON) - .setRefreshPolicy(IMMEDIATE) - .setRouting(routing) - .get(); - - checkAccountingBreaker(); - // Test that force merging causes the breaker to be correctly adjusted - logger.info("--> force merging to a single segment"); - client().admin().indices().prepareForceMerge("test").setMaxNumSegments(1).setFlush(randomBoolean()).get(); - client().admin().indices().prepareRefresh().get(); - checkAccountingBreaker(); - - client().admin() - .cluster() - .prepareUpdateSettings() - .setTransientSettings(Settings.builder().put("indices.breaker.total.limit", "1kb")) - .get(); - - // Test that we're now above the parent limit due to the segments - Exception e = expectThrows( - Exception.class, - () -> client().prepareSearch("test").addAggregation(AggregationBuilders.terms("foo_terms").field("foo.keyword")).get() - ); - logger.info("--> got an expected exception", e); - assertThat(e.getCause(), notNullValue()); - assertThat(e.getCause().getMessage(), containsString("[parent] Data too large, data for []")); - - client().admin() - .cluster() - .prepareUpdateSettings() - .setTransientSettings( - Settings.builder().putNull("indices.breaker.total.limit").putNull("network.breaker.inflight_requests.overhead") - ) - .get(); - - // Test that deleting the index causes the breaker to correctly be decremented - logger.info("--> deleting index"); - client().admin().indices().prepareDelete("test").get(); - - // Accounting breaker should now be 0 - CircuitBreakerService breakerService = getInstanceFromNode(CircuitBreakerService.class); - CircuitBreaker acctBreaker = breakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(acctBreaker.getUsed(), equalTo(0L)); - } - public static final IndexShard recoverShard(IndexShard newShard) throws IOException { DiscoveryNode localNode = new DiscoveryNode("foo", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT); newShard.markAsRecovering("store", new RecoveryState(newShard.routingEntry(), localNode, null)); diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/stats/IndexStatsIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/stats/IndexStatsIT.java index b247c9b3f397b..9cd91cab2b122 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/stats/IndexStatsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/stats/IndexStatsIT.java @@ -828,7 +828,6 @@ public void testSegmentsStats() { assertThat(stats.getTotal().getSegments(), notNullValue()); assertThat(stats.getTotal().getSegments().getCount(), equalTo((long) test1.totalNumShards)); - assertThat(stats.getTotal().getSegments().getMemoryInBytes(), greaterThan(0L)); } public void testAllFlags() throws Exception { diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java index d3b8e4c2eb1b8..ed9c086d0481c 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java @@ -139,7 +139,7 @@ protected void addCustomXContentFields(XContentBuilder builder, Params params) t builder.field(Fields.NUM_DOCS, segment.getNumDocs()); builder.field(Fields.DELETED_DOCS, segment.getDeletedDocs()); builder.humanReadableField(Fields.SIZE_IN_BYTES, Fields.SIZE, segment.getSize()); - builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, new ByteSizeValue(segment.getMemoryInBytes())); + builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, segment.getZeroMemory()); builder.field(Fields.COMMITTED, segment.isCommitted()); builder.field(Fields.SEARCH, segment.isSearch()); if (segment.getVersion() != null) { diff --git a/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStats.java b/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStats.java index c09abd595cb73..015d52f15f907 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStats.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStats.java @@ -493,7 +493,7 @@ public RecoveryStats getRecoveryStats() { /** * Utility method which computes total memory by adding - * FieldData, PercolatorCache, Segments (memory, index writer, version map) + * FieldData, PercolatorCache, Segments (index writer, version map) */ public ByteSizeValue getTotalMemory() { long size = 0; @@ -504,8 +504,7 @@ public ByteSizeValue getTotalMemory() { size += this.getQueryCache().getMemorySizeInBytes(); } if (this.getSegments() != null) { - size += this.getSegments().getMemoryInBytes() + this.getSegments().getIndexWriterMemoryInBytes() + this.getSegments() - .getVersionMapMemoryInBytes(); + size += this.getSegments().getIndexWriterMemoryInBytes() + this.getSegments().getVersionMapMemoryInBytes(); } return new ByteSizeValue(size); diff --git a/server/src/main/java/org/opensearch/index/engine/Engine.java b/server/src/main/java/org/opensearch/index/engine/Engine.java index 3e92f6a2aef97..1f330990348dc 100644 --- a/server/src/main/java/org/opensearch/index/engine/Engine.java +++ b/server/src/main/java/org/opensearch/index/engine/Engine.java @@ -51,7 +51,6 @@ import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; import org.apache.lucene.store.IOContext; -import org.apache.lucene.util.Accountable; import org.apache.lucene.util.Accountables; import org.apache.lucene.util.SetOnce; import org.opensearch.ExceptionsHelper; @@ -161,14 +160,6 @@ protected Engine(EngineConfig engineConfig) { this.eventListener = engineConfig.getEventListener(); } - /** Returns 0 in the case where accountable is null, otherwise returns {@code ramBytesUsed()} */ - protected static long guardedRamBytesUsed(Accountable a) { - if (a == null) { - return 0; - } - return a.ramBytesUsed(); - } - public final EngineConfig config() { return engineConfig; } @@ -875,14 +866,7 @@ public SegmentsStats segmentsStats(boolean includeSegmentFileSizes, boolean incl } protected void fillSegmentStats(SegmentReader segmentReader, boolean includeSegmentFileSizes, SegmentsStats stats) { - stats.add(1, segmentReader.ramBytesUsed()); - stats.addTermsMemoryInBytes(guardedRamBytesUsed(segmentReader.getPostingsReader())); - stats.addStoredFieldsMemoryInBytes(guardedRamBytesUsed(segmentReader.getFieldsReader())); - stats.addTermVectorsMemoryInBytes(guardedRamBytesUsed(segmentReader.getTermVectorsReader())); - stats.addNormsMemoryInBytes(guardedRamBytesUsed(segmentReader.getNormsReader())); - stats.addPointsMemoryInBytes(guardedRamBytesUsed(segmentReader.getPointsReader())); - stats.addDocValuesMemoryInBytes(guardedRamBytesUsed(segmentReader.getDocValuesReader())); - + stats.add(1); if (includeSegmentFileSizes) { // TODO: consider moving this to StoreStats stats.addFileSizes(getSegmentFileSizes(segmentReader)); @@ -1048,7 +1032,6 @@ private void fillSegmentInfo(SegmentReader segmentReader, boolean verbose, boole } catch (IOException e) { logger.trace(() -> new ParameterizedMessage("failed to get size for [{}]", info.info.name), e); } - segment.memoryInBytes = segmentReader.ramBytesUsed(); segment.segmentSort = info.info.getIndexSort(); if (verbose) { segment.ramTree = Accountables.namedAccountable("root", segmentReader); diff --git a/server/src/main/java/org/opensearch/index/engine/InternalEngine.java b/server/src/main/java/org/opensearch/index/engine/InternalEngine.java index 955ddcef4869d..ae508d627a00a 100644 --- a/server/src/main/java/org/opensearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/opensearch/index/engine/InternalEngine.java @@ -701,10 +701,7 @@ private ExternalReaderManager createReaderManager(RefreshWarmerListener external DirectoryReader.open(indexWriter), shardId ); - internalReaderManager = new OpenSearchReaderManager( - directoryReader, - new RamAccountingRefreshListener(engineConfig.getCircuitBreakerService()) - ); + internalReaderManager = new OpenSearchReaderManager(directoryReader); lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo(); ExternalReaderManager externalReaderManager = new ExternalReaderManager(internalReaderManager, externalRefreshListener); success = true; diff --git a/server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java b/server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java index d68183f9cd07a..b83ce8bea567c 100644 --- a/server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java +++ b/server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java @@ -33,7 +33,6 @@ package org.opensearch.index.engine; import java.io.IOException; -import java.util.function.BiConsumer; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.search.ReferenceManager; @@ -52,23 +51,15 @@ */ @SuppressForbidden(reason = "reference counting is required here") class OpenSearchReaderManager extends ReferenceManager { - private final BiConsumer refreshListener; - /** * Creates and returns a new OpenSearchReaderManager from the given * already-opened {@link OpenSearchDirectoryReader}, stealing * the incoming reference. * * @param reader the directoryReader to use for future reopens - * @param refreshListener A consumer that is called every time a new reader is opened */ - OpenSearchReaderManager( - OpenSearchDirectoryReader reader, - BiConsumer refreshListener - ) { + OpenSearchReaderManager(OpenSearchDirectoryReader reader) { this.current = reader; - this.refreshListener = refreshListener; - refreshListener.accept(current, null); } @Override @@ -79,9 +70,6 @@ protected void decRef(OpenSearchDirectoryReader reference) throws IOException { @Override protected OpenSearchDirectoryReader refreshIfNeeded(OpenSearchDirectoryReader referenceToRefresh) throws IOException { final OpenSearchDirectoryReader reader = (OpenSearchDirectoryReader) DirectoryReader.openIfChanged(referenceToRefresh); - if (reader != null) { - refreshListener.accept(reader, referenceToRefresh); - } return reader; } diff --git a/server/src/main/java/org/opensearch/index/engine/RamAccountingRefreshListener.java b/server/src/main/java/org/opensearch/index/engine/RamAccountingRefreshListener.java deleted file mode 100644 index beba2ff25304e..0000000000000 --- a/server/src/main/java/org/opensearch/index/engine/RamAccountingRefreshListener.java +++ /dev/null @@ -1,94 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -/* - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.index.engine; - -import org.apache.lucene.index.IndexReader; -import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.index.SegmentReader; -import org.opensearch.common.breaker.CircuitBreaker; -import org.opensearch.common.lucene.Lucene; -import org.opensearch.common.lucene.index.OpenSearchDirectoryReader; -import org.opensearch.indices.breaker.CircuitBreakerService; - -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.function.BiConsumer; - -/** - * A refresh listener that tracks the amount of memory used by segments in the accounting circuit breaker. - */ -final class RamAccountingRefreshListener implements BiConsumer { - - private final CircuitBreakerService breakerService; - - RamAccountingRefreshListener(CircuitBreakerService breakerService) { - this.breakerService = breakerService; - } - - @Override - public void accept(OpenSearchDirectoryReader reader, OpenSearchDirectoryReader previousReader) { - final CircuitBreaker breaker = breakerService.getBreaker(CircuitBreaker.ACCOUNTING); - - // Construct a list of the previous segment readers, we only want to track memory used - // by new readers, so these will be exempted from the circuit breaking accounting. - // - // The Core CacheKey is used as the key for the set so that deletions still keep the correct - // accounting, as using the Reader or Reader's CacheKey causes incorrect accounting. - final Set prevReaders; - if (previousReader == null) { - prevReaders = Collections.emptySet(); - } else { - final List previousReaderLeaves = previousReader.leaves(); - prevReaders = new HashSet<>(previousReaderLeaves.size()); - for (LeafReaderContext lrc : previousReaderLeaves) { - prevReaders.add(Lucene.segmentReader(lrc.reader()).getCoreCacheHelper().getKey()); - } - } - - for (LeafReaderContext lrc : reader.leaves()) { - final SegmentReader segmentReader = Lucene.segmentReader(lrc.reader()); - // don't add the segment's memory unless it is not referenced by the previous reader - // (only new segments) - if (prevReaders.contains(segmentReader.getCoreCacheHelper().getKey()) == false) { - final long ramBytesUsed = segmentReader.ramBytesUsed(); - // add the segment memory to the breaker (non-breaking) - breaker.addWithoutBreaking(ramBytesUsed); - // and register a listener for when the segment is closed to decrement the - // breaker accounting - segmentReader.getCoreCacheHelper().addClosedListener(k -> breaker.addWithoutBreaking(-ramBytesUsed)); - } - } - } -} diff --git a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java index b32618df7932c..0491eb0db94cd 100644 --- a/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java +++ b/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java @@ -85,7 +85,6 @@ public class ReadOnlyEngine extends Engine { private final OpenSearchReaderManager readerManager; private final IndexCommit indexCommit; private final Lock indexWriterLock; - private final RamAccountingRefreshListener refreshListener; private final SafeCommitInfo safeCommitInfo; private final CompletionStatsCache completionStatsCache; private final boolean requireCompleteHistory; @@ -114,7 +113,6 @@ public ReadOnlyEngine( boolean requireCompleteHistory ) { super(config); - this.refreshListener = new RamAccountingRefreshListener(engineConfig.getCircuitBreakerService()); this.requireCompleteHistory = requireCompleteHistory; try { Store store = config.getStore(); @@ -135,14 +133,13 @@ public ReadOnlyEngine( this.seqNoStats = seqNoStats; this.indexCommit = Lucene.getIndexCommit(lastCommittedSegmentInfos, directory); reader = wrapReader(open(indexCommit), readerWrapperFunction); - readerManager = new OpenSearchReaderManager(reader, refreshListener); + readerManager = new OpenSearchReaderManager(reader); assert translogStats != null || obtainLock : "mutiple translogs instances should not be opened at the same time"; this.translogStats = translogStats != null ? translogStats : translogStats(config, lastCommittedSegmentInfos); this.indexWriterLock = indexWriterLock; this.safeCommitInfo = new SafeCommitInfo(seqNoStats.getLocalCheckpoint(), lastCommittedSegmentInfos.totalMaxDoc()); completionStatsCache = new CompletionStatsCache(() -> acquireSearcher("completion_stats")); - // no need to register a refresh listener to invalidate completionStatsCache since this engine is readonly success = true; } finally { @@ -511,10 +508,6 @@ public void updateMaxUnsafeAutoIdTimestamp(long newTimestamp) { } - protected void processReader(OpenSearchDirectoryReader reader) { - refreshListener.accept(reader, null); - } - @Override public boolean refreshNeeded() { return false; diff --git a/server/src/main/java/org/opensearch/index/engine/Segment.java b/server/src/main/java/org/opensearch/index/engine/Segment.java index 1ef3a2f94a8e1..5fdff0e52fc5c 100644 --- a/server/src/main/java/org/opensearch/index/engine/Segment.java +++ b/server/src/main/java/org/opensearch/index/engine/Segment.java @@ -40,6 +40,7 @@ import org.apache.lucene.search.SortedNumericSelector; import org.apache.lucene.util.Accountable; import org.apache.lucene.util.Accountables; +import org.opensearch.Version; import org.opensearch.common.Nullable; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; @@ -66,7 +67,6 @@ public class Segment implements Writeable { public org.apache.lucene.util.Version version = null; public Boolean compound = null; public String mergeId; - public long memoryInBytes; public Sort segmentSort; public Accountable ramTree = null; public Map attributes; @@ -82,7 +82,11 @@ public Segment(StreamInput in) throws IOException { version = Lucene.parseVersionLenient(in.readOptionalString(), null); compound = in.readOptionalBoolean(); mergeId = in.readOptionalString(); - memoryInBytes = in.readLong(); + // the following was removed in Lucene 9 (https://issues.apache.org/jira/browse/LUCENE-9387) + // retain for bwc only (todo: remove in OpenSearch 3) + if (in.getVersion().before(Version.V_2_0_0)) { + in.readLong(); // estimated memory + } if (in.readBoolean()) { // verbose mode ramTree = readRamTree(in); @@ -145,10 +149,13 @@ public String getMergeId() { } /** - * Estimation of the memory usage used by a segment. + * Estimation of the memory usage was removed in Lucene 9 (https://issues.apache.org/jira/browse/LUCENE-9387) + * retain for bwc only (todo: remove in OpenSearch 3). + * @deprecated */ - public long getMemoryInBytes() { - return this.memoryInBytes; + @Deprecated + public long getZeroMemory() { + return 0L; } /** @@ -193,7 +200,11 @@ public void writeTo(StreamOutput out) throws IOException { out.writeOptionalString(version.toString()); out.writeOptionalBoolean(compound); out.writeOptionalString(mergeId); - out.writeLong(memoryInBytes); + // the following was removed in Lucene 9 (https://issues.apache.org/jira/browse/LUCENE-9387) + // retain for bwc only (todo: remove in OpenSearch 3) + if (out.getVersion().before(Version.V_2_0_0)) { + out.writeLong(0L); + } boolean verbose = ramTree != null; out.writeBoolean(verbose); @@ -350,8 +361,6 @@ public String toString() { + ", mergeId='" + mergeId + '\'' - + ", memoryInBytes=" - + memoryInBytes + (segmentSort != null ? ", sort=" + segmentSort : "") + ", attributes=" + attributes diff --git a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java index 0db8ca7b94425..e1f52ae86d27d 100644 --- a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java +++ b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java @@ -33,6 +33,7 @@ package org.opensearch.index.engine; import com.carrotsearch.hppc.cursors.ObjectObjectCursor; +import org.opensearch.Version; import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; @@ -46,13 +47,6 @@ public class SegmentsStats implements Writeable, ToXContentFragment { private long count; - private long memoryInBytes; - private long termsMemoryInBytes; - private long storedFieldsMemoryInBytes; - private long termVectorsMemoryInBytes; - private long normsMemoryInBytes; - private long pointsMemoryInBytes; - private long docValuesMemoryInBytes; private long indexWriterMemoryInBytes; private long versionMapMemoryInBytes; private long maxUnsafeAutoIdTimestamp = Long.MIN_VALUE; @@ -91,13 +85,17 @@ public SegmentsStats() {} public SegmentsStats(StreamInput in) throws IOException { count = in.readVLong(); - memoryInBytes = in.readLong(); - termsMemoryInBytes = in.readLong(); - storedFieldsMemoryInBytes = in.readLong(); - termVectorsMemoryInBytes = in.readLong(); - normsMemoryInBytes = in.readLong(); - pointsMemoryInBytes = in.readLong(); - docValuesMemoryInBytes = in.readLong(); + // the following was removed in Lucene 9 (https://issues.apache.org/jira/browse/LUCENE-9387) + // retain for bwc only (todo: remove in OpenSearch 3) + if (in.getVersion().before(Version.V_2_0_0)) { + in.readLong(); // estimated segment memory + in.readLong(); // estimated terms memory + in.readLong(); // estimated stored fields memory + in.readLong(); // estimated term vector memory + in.readLong(); // estimated norms memory + in.readLong(); // estimated points memory + in.readLong(); // estimated doc values memory + } indexWriterMemoryInBytes = in.readLong(); versionMapMemoryInBytes = in.readLong(); bitsetMemoryInBytes = in.readLong(); @@ -113,33 +111,8 @@ public SegmentsStats(StreamInput in) throws IOException { fileSizes = map.build(); } - public void add(long count, long memoryInBytes) { + public void add(long count) { this.count += count; - this.memoryInBytes += memoryInBytes; - } - - public void addTermsMemoryInBytes(long termsMemoryInBytes) { - this.termsMemoryInBytes += termsMemoryInBytes; - } - - public void addStoredFieldsMemoryInBytes(long storedFieldsMemoryInBytes) { - this.storedFieldsMemoryInBytes += storedFieldsMemoryInBytes; - } - - public void addTermVectorsMemoryInBytes(long termVectorsMemoryInBytes) { - this.termVectorsMemoryInBytes += termVectorsMemoryInBytes; - } - - public void addNormsMemoryInBytes(long normsMemoryInBytes) { - this.normsMemoryInBytes += normsMemoryInBytes; - } - - public void addPointsMemoryInBytes(long pointsMemoryInBytes) { - this.pointsMemoryInBytes += pointsMemoryInBytes; - } - - public void addDocValuesMemoryInBytes(long docValuesMemoryInBytes) { - this.docValuesMemoryInBytes += docValuesMemoryInBytes; } public void addIndexWriterMemoryInBytes(long indexWriterMemoryInBytes) { @@ -178,13 +151,7 @@ public void add(SegmentsStats mergeStats) { return; } updateMaxUnsafeAutoIdTimestamp(mergeStats.maxUnsafeAutoIdTimestamp); - add(mergeStats.count, mergeStats.memoryInBytes); - addTermsMemoryInBytes(mergeStats.termsMemoryInBytes); - addStoredFieldsMemoryInBytes(mergeStats.storedFieldsMemoryInBytes); - addTermVectorsMemoryInBytes(mergeStats.termVectorsMemoryInBytes); - addNormsMemoryInBytes(mergeStats.normsMemoryInBytes); - addPointsMemoryInBytes(mergeStats.pointsMemoryInBytes); - addDocValuesMemoryInBytes(mergeStats.docValuesMemoryInBytes); + add(mergeStats.count); addIndexWriterMemoryInBytes(mergeStats.indexWriterMemoryInBytes); addVersionMapMemoryInBytes(mergeStats.versionMapMemoryInBytes); addBitsetMemoryInBytes(mergeStats.bitsetMemoryInBytes); @@ -198,83 +165,6 @@ public long getCount() { return this.count; } - /** - * Estimation of the memory usage used by a segment. - */ - public long getMemoryInBytes() { - return this.memoryInBytes; - } - - public ByteSizeValue getMemory() { - return new ByteSizeValue(memoryInBytes); - } - - /** - * Estimation of the terms dictionary memory usage by a segment. - */ - public long getTermsMemoryInBytes() { - return this.termsMemoryInBytes; - } - - private ByteSizeValue getTermsMemory() { - return new ByteSizeValue(termsMemoryInBytes); - } - - /** - * Estimation of the stored fields memory usage by a segment. - */ - public long getStoredFieldsMemoryInBytes() { - return this.storedFieldsMemoryInBytes; - } - - private ByteSizeValue getStoredFieldsMemory() { - return new ByteSizeValue(storedFieldsMemoryInBytes); - } - - /** - * Estimation of the term vectors memory usage by a segment. - */ - public long getTermVectorsMemoryInBytes() { - return this.termVectorsMemoryInBytes; - } - - private ByteSizeValue getTermVectorsMemory() { - return new ByteSizeValue(termVectorsMemoryInBytes); - } - - /** - * Estimation of the norms memory usage by a segment. - */ - public long getNormsMemoryInBytes() { - return this.normsMemoryInBytes; - } - - private ByteSizeValue getNormsMemory() { - return new ByteSizeValue(normsMemoryInBytes); - } - - /** - * Estimation of the points memory usage by a segment. - */ - public long getPointsMemoryInBytes() { - return this.pointsMemoryInBytes; - } - - private ByteSizeValue getPointsMemory() { - return new ByteSizeValue(pointsMemoryInBytes); - } - - /** - * Estimation of the doc values memory usage by a segment. - */ - public long getDocValuesMemoryInBytes() { - return this.docValuesMemoryInBytes; - } - - private ByteSizeValue getDocValuesMemory() { - return new ByteSizeValue(docValuesMemoryInBytes); - } - /** * Estimation of the memory usage by index writer */ @@ -324,13 +214,13 @@ public long getMaxUnsafeAutoIdTimestamp() { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(Fields.SEGMENTS); builder.field(Fields.COUNT, count); - builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, getMemory()); - builder.humanReadableField(Fields.TERMS_MEMORY_IN_BYTES, Fields.TERMS_MEMORY, getTermsMemory()); - builder.humanReadableField(Fields.STORED_FIELDS_MEMORY_IN_BYTES, Fields.STORED_FIELDS_MEMORY, getStoredFieldsMemory()); - builder.humanReadableField(Fields.TERM_VECTORS_MEMORY_IN_BYTES, Fields.TERM_VECTORS_MEMORY, getTermVectorsMemory()); - builder.humanReadableField(Fields.NORMS_MEMORY_IN_BYTES, Fields.NORMS_MEMORY, getNormsMemory()); - builder.humanReadableField(Fields.POINTS_MEMORY_IN_BYTES, Fields.POINTS_MEMORY, getPointsMemory()); - builder.humanReadableField(Fields.DOC_VALUES_MEMORY_IN_BYTES, Fields.DOC_VALUES_MEMORY, getDocValuesMemory()); + builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, 0L); + builder.humanReadableField(Fields.TERMS_MEMORY_IN_BYTES, Fields.TERMS_MEMORY, 0L); + builder.humanReadableField(Fields.STORED_FIELDS_MEMORY_IN_BYTES, Fields.STORED_FIELDS_MEMORY, 0L); + builder.humanReadableField(Fields.TERM_VECTORS_MEMORY_IN_BYTES, Fields.TERM_VECTORS_MEMORY, 0L); + builder.humanReadableField(Fields.NORMS_MEMORY_IN_BYTES, Fields.NORMS_MEMORY, 0L); + builder.humanReadableField(Fields.POINTS_MEMORY_IN_BYTES, Fields.POINTS_MEMORY, 0L); + builder.humanReadableField(Fields.DOC_VALUES_MEMORY_IN_BYTES, Fields.DOC_VALUES_MEMORY, 0L); builder.humanReadableField(Fields.INDEX_WRITER_MEMORY_IN_BYTES, Fields.INDEX_WRITER_MEMORY, getIndexWriterMemory()); builder.humanReadableField(Fields.VERSION_MAP_MEMORY_IN_BYTES, Fields.VERSION_MAP_MEMORY, getVersionMapMemory()); builder.humanReadableField(Fields.FIXED_BIT_SET_MEMORY_IN_BYTES, Fields.FIXED_BIT_SET, getBitsetMemory()); @@ -380,13 +270,17 @@ static final class Fields { @Override public void writeTo(StreamOutput out) throws IOException { out.writeVLong(count); - out.writeLong(memoryInBytes); - out.writeLong(termsMemoryInBytes); - out.writeLong(storedFieldsMemoryInBytes); - out.writeLong(termVectorsMemoryInBytes); - out.writeLong(normsMemoryInBytes); - out.writeLong(pointsMemoryInBytes); - out.writeLong(docValuesMemoryInBytes); + if (out.getVersion().before(Version.V_2_0_0)) { + // the following was removed in Lucene 9 (https://issues.apache.org/jira/browse/LUCENE-9387) + // retain the following for bwc only (todo: remove in OpenSearch 3) + out.writeLong(0L); // estimated memory + out.writeLong(0L); // estimated terms memory + out.writeLong(0L); // estimated stored fields memory + out.writeLong(0L); // estimated term vector memory + out.writeLong(0L); // estimated norms memory + out.writeLong(0L); // estimated points memory + out.writeLong(0L); // estimated doc values memory + } out.writeLong(indexWriterMemoryInBytes); out.writeLong(versionMapMemoryInBytes); out.writeLong(bitsetMemoryInBytes); @@ -402,4 +296,14 @@ public void writeTo(StreamOutput out) throws IOException { public void clearFileSizes() { fileSizes = ImmutableOpenMap.of(); } + + /** + * Used only for deprecating memory tracking in REST interface + * todo remove in OpenSearch 3.0 + * @deprecated + */ + @Deprecated + public Long getZeroMemory() { + return 0L; + } } diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java index 80c42da5c4b83..2c0eef6a8fdb8 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestIndicesAction.java @@ -850,8 +850,8 @@ Table buildTable( table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getCount()); table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getCount()); - table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getMemory()); - table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getMemory()); + table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getZeroMemory()); + table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getZeroMemory()); table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getIndexWriterMemory()); table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getIndexWriterMemory()); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java index 30970511e865e..bce9b2d6b7e9d 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestNodesAction.java @@ -501,7 +501,7 @@ Table buildTable( SegmentsStats segmentsStats = indicesStats == null ? null : indicesStats.getSegments(); table.addCell(segmentsStats == null ? null : segmentsStats.getCount()); - table.addCell(segmentsStats == null ? null : segmentsStats.getMemory()); + table.addCell(segmentsStats == null ? null : segmentsStats.getZeroMemory()); table.addCell(segmentsStats == null ? null : segmentsStats.getIndexWriterMemory()); table.addCell(segmentsStats == null ? null : segmentsStats.getVersionMapMemory()); table.addCell(segmentsStats == null ? null : segmentsStats.getBitsetMemory()); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java index c258825e669b4..f2e0e49fe97b5 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java @@ -155,7 +155,7 @@ private Table buildTable(final RestRequest request, ClusterStateResponse state, table.addCell(segment.getNumDocs()); table.addCell(segment.getDeletedDocs()); table.addCell(segment.getSize()); - table.addCell(segment.getMemoryInBytes()); + table.addCell(segment.getZeroMemory()); table.addCell(segment.isCommitted()); table.addCell(segment.isSearch()); table.addCell(segment.getVersion()); diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestShardsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestShardsAction.java index eea5da25776e9..f9aa1a5554e9e 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestShardsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestShardsAction.java @@ -381,7 +381,7 @@ Table buildTable(RestRequest request, ClusterStateResponse state, IndicesStatsRe table.addCell(getOrNull(commonStats, CommonStats::getSearch, i -> i.getTotal().getScrollCount())); table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getCount)); - table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getMemory)); + table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getZeroMemory)); table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getIndexWriterMemory)); table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getVersionMapMemory)); table.addCell(getOrNull(commonStats, CommonStats::getSegments, SegmentsStats::getBitsetMemory)); diff --git a/server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsTests.java b/server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsTests.java index 869928e216815..0cf9f9fe152d6 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsTests.java @@ -60,12 +60,7 @@ public void testSegmentStatsEmptyIndex() { createIndex("test"); IndicesStatsResponse rsp = client().admin().indices().prepareStats("test").get(); SegmentsStats stats = rsp.getTotal().getSegments(); - assertEquals(0, stats.getTermsMemoryInBytes()); - assertEquals(0, stats.getStoredFieldsMemoryInBytes()); - assertEquals(0, stats.getTermVectorsMemoryInBytes()); - assertEquals(0, stats.getNormsMemoryInBytes()); - assertEquals(0, stats.getPointsMemoryInBytes()); - assertEquals(0, stats.getDocValuesMemoryInBytes()); + assertEquals(0, stats.getCount()); } public void testSegmentStats() throws Exception { @@ -102,16 +97,8 @@ public void testSegmentStats() throws Exception { IndicesStatsResponse rsp = client().admin().indices().prepareStats("test").get(); SegmentsStats stats = rsp.getIndex("test").getTotal().getSegments(); - assertThat(stats.getTermsMemoryInBytes(), greaterThan(0L)); - assertThat(stats.getStoredFieldsMemoryInBytes(), greaterThan(0L)); - assertThat(stats.getTermVectorsMemoryInBytes(), greaterThan(0L)); - assertThat(stats.getNormsMemoryInBytes(), greaterThan(0L)); - assertThat(stats.getDocValuesMemoryInBytes(), greaterThan(0L)); - if ((storeType == IndexModule.Type.MMAPFS) || (storeType == IndexModule.Type.HYBRIDFS)) { - assertEquals(0, stats.getPointsMemoryInBytes()); // bkd tree is stored off-heap - } else { - assertThat(stats.getPointsMemoryInBytes(), greaterThan(0L)); // bkd tree is stored on heap - } + // should be more than one segment since data was indexed + assertThat(stats.getCount(), greaterThan(0L)); // now check multiple segments stats are merged together client().prepareIndex("test", "doc", "2").setSource("foo", "bar", "bar", "baz", "baz", 43).get(); @@ -119,16 +106,8 @@ public void testSegmentStats() throws Exception { rsp = client().admin().indices().prepareStats("test").get(); SegmentsStats stats2 = rsp.getIndex("test").getTotal().getSegments(); - assertThat(stats2.getTermsMemoryInBytes(), greaterThan(stats.getTermsMemoryInBytes())); - assertThat(stats2.getStoredFieldsMemoryInBytes(), greaterThan(stats.getStoredFieldsMemoryInBytes())); - assertThat(stats2.getTermVectorsMemoryInBytes(), greaterThan(stats.getTermVectorsMemoryInBytes())); - assertThat(stats2.getNormsMemoryInBytes(), greaterThan(stats.getNormsMemoryInBytes())); - assertThat(stats2.getDocValuesMemoryInBytes(), greaterThan(stats.getDocValuesMemoryInBytes())); - if ((storeType == IndexModule.Type.MMAPFS) || (storeType == IndexModule.Type.HYBRIDFS)) { - assertEquals(0, stats2.getPointsMemoryInBytes()); // bkd tree is stored off-heap - } else { - assertThat(stats2.getPointsMemoryInBytes(), greaterThan(stats.getPointsMemoryInBytes())); // bkd tree is stored on heap - } + // stats2 should exceed stats since multiple segments stats were merged + assertThat(stats2.getCount(), greaterThan(stats.getCount())); } public void testCommitStats() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java index d20b151bb69bf..928dad8685cfe 100644 --- a/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java @@ -98,7 +98,6 @@ import org.opensearch.common.Strings; import org.opensearch.common.TriFunction; import org.opensearch.common.UUIDs; -import org.opensearch.common.breaker.CircuitBreaker; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.collect.Tuple; @@ -6712,7 +6711,6 @@ public void testRefreshAndCloseEngineConcurrently() throws Exception { indexer.join(); refresher.join(); } - assertThat(engine.config().getCircuitBreakerService().getBreaker(CircuitBreaker.ACCOUNTING).getUsed(), equalTo(0L)); } public void testPruneAwayDeletedButRetainedIds() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/engine/NoOpEngineTests.java b/server/src/test/java/org/opensearch/index/engine/NoOpEngineTests.java index d46563ff09ccb..65b8a81b029c0 100644 --- a/server/src/test/java/org/opensearch/index/engine/NoOpEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/NoOpEngineTests.java @@ -182,7 +182,6 @@ public void testNoOpEngineStats() throws Exception { ); assertEquals(0, noOpEngine.segmentsStats(includeFileSize, false).getFileSizes().size()); - assertEquals(0, noOpEngine.segmentsStats(includeFileSize, false).getMemoryInBytes()); } catch (AssertionError e) { logger.error(config.getMergePolicy()); throw e; diff --git a/server/src/test/java/org/opensearch/index/engine/SegmentTests.java b/server/src/test/java/org/opensearch/index/engine/SegmentTests.java index 103e71c5ddc75..744b0d0cb4733 100644 --- a/server/src/test/java/org/opensearch/index/engine/SegmentTests.java +++ b/server/src/test/java/org/opensearch/index/engine/SegmentTests.java @@ -95,7 +95,6 @@ static Segment randomSegment() { segment.version = Version.LUCENE_7_0_0; segment.compound = randomBoolean(); segment.mergeId = randomAlphaOfLengthBetween(1, 10); - segment.memoryInBytes = randomNonNegativeLong(); segment.segmentSort = randomIndexSort(); if (randomBoolean()) { segment.attributes = Collections.singletonMap("foo", "bar"); @@ -123,7 +122,6 @@ static boolean isSegmentEquals(Segment seg1, Segment seg2) { && Objects.equals(seg1.version, seg2.version) && Objects.equals(seg1.compound, seg2.compound) && seg1.sizeInBytes == seg2.sizeInBytes - && seg1.memoryInBytes == seg2.memoryInBytes && seg1.getGeneration() == seg2.getGeneration() && seg1.getName().equals(seg2.getName()) && seg1.getMergeId().equals(seg2.getMergeId()) diff --git a/server/src/test/java/org/opensearch/index/shard/IndexShardTests.java b/server/src/test/java/org/opensearch/index/shard/IndexShardTests.java index 2575258b28968..0c771a46ab226 100644 --- a/server/src/test/java/org/opensearch/index/shard/IndexShardTests.java +++ b/server/src/test/java/org/opensearch/index/shard/IndexShardTests.java @@ -70,7 +70,6 @@ import org.opensearch.common.Randomness; import org.opensearch.common.Strings; import org.opensearch.common.UUIDs; -import org.opensearch.common.breaker.CircuitBreaker; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.stream.BytesStreamOutput; @@ -88,7 +87,6 @@ import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.internal.io.IOUtils; import org.opensearch.env.NodeEnvironment; import org.opensearch.index.IndexSettings; import org.opensearch.index.VersionType; @@ -103,7 +101,6 @@ import org.opensearch.index.engine.InternalEngine; import org.opensearch.index.engine.InternalEngineFactory; import org.opensearch.index.engine.ReadOnlyEngine; -import org.opensearch.index.engine.SegmentsStats; import org.opensearch.index.fielddata.FieldDataStats; import org.opensearch.index.fielddata.IndexFieldData; import org.opensearch.index.fielddata.IndexFieldDataCache; @@ -201,7 +198,6 @@ import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; @@ -3916,172 +3912,6 @@ public void testRefreshIsNeededWithRefreshListeners() throws IOException, Interr closeShards(primary); } - public void testSegmentMemoryTrackedInBreaker() throws Exception { - Settings settings = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .build(); - IndexMetadata metadata = IndexMetadata.builder("test") - .putMapping("_doc", "{ \"properties\": { \"foo\": { \"type\": \"text\"}}}") - .settings(settings) - .primaryTerm(0, 1) - .build(); - IndexShard primary = newShard(new ShardId(metadata.getIndex(), 0), true, "n1", metadata, null); - recoverShardFromStore(primary); - indexDoc(primary, "_doc", "0", "{\"foo\" : \"foo\"}"); - primary.refresh("forced refresh"); - - SegmentsStats ss = primary.segmentStats(randomBoolean(), randomBoolean()); - CircuitBreaker breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(ss.getMemoryInBytes(), equalTo(breaker.getUsed())); - final long preRefreshBytes = ss.getMemoryInBytes(); - - indexDoc(primary, "_doc", "1", "{\"foo\" : \"bar\"}"); - indexDoc(primary, "_doc", "2", "{\"foo\" : \"baz\"}"); - indexDoc(primary, "_doc", "3", "{\"foo\" : \"eggplant\"}"); - - ss = primary.segmentStats(randomBoolean(), randomBoolean()); - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(preRefreshBytes, equalTo(breaker.getUsed())); - - primary.refresh("refresh"); - - ss = primary.segmentStats(randomBoolean(), randomBoolean()); - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(breaker.getUsed(), equalTo(ss.getMemoryInBytes())); - assertThat(breaker.getUsed(), greaterThan(preRefreshBytes)); - - indexDoc(primary, "_doc", "4", "{\"foo\": \"potato\"}"); - indexDoc(primary, "_doc", "5", "{\"foo\": \"potato\"}"); - // Forces a refresh with the INTERNAL scope - ((InternalEngine) primary.getEngine()).writeIndexingBuffer(); - - ss = primary.segmentStats(randomBoolean(), randomBoolean()); - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(breaker.getUsed(), equalTo(ss.getMemoryInBytes())); - assertThat(breaker.getUsed(), greaterThan(preRefreshBytes)); - final long postRefreshBytes = ss.getMemoryInBytes(); - - // Deleting a doc causes its memory to be freed from the breaker - deleteDoc(primary, "_doc", "0"); - // Here we are testing that a fully deleted segment should be dropped and its memory usage is freed. - // In order to instruct the merge policy not to keep a fully deleted segment, - // we need to flush and make that commit safe so that the SoftDeletesPolicy can drop everything. - primary.updateGlobalCheckpointForShard(primary.routingEntry().allocationId().getId(), primary.getLastSyncedGlobalCheckpoint()); - primary.syncRetentionLeases(); - primary.sync(); - flushShard(primary); - primary.refresh("force refresh"); - - ss = primary.segmentStats(randomBoolean(), randomBoolean()); - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(breaker.getUsed(), lessThan(postRefreshBytes)); - - closeShards(primary); - - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(breaker.getUsed(), equalTo(0L)); - } - - public void testSegmentMemoryTrackedWithRandomSearchers() throws Exception { - Settings settings = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .build(); - IndexMetadata metadata = IndexMetadata.builder("test") - .putMapping("_doc", "{ \"properties\": { \"foo\": { \"type\": \"text\"}}}") - .settings(settings) - .primaryTerm(0, 1) - .build(); - IndexShard primary = newShard(new ShardId(metadata.getIndex(), 0), true, "n1", metadata, null); - recoverShardFromStore(primary); - - int threadCount = randomIntBetween(2, 4); - List threads = new ArrayList<>(threadCount); - int iterations = randomIntBetween(10, 20); - List searchers = Collections.synchronizedList(new ArrayList<>()); - - logger.info("--> running with {} threads and {} iterations each", threadCount, iterations); - for (int threadId = 0; threadId < threadCount; threadId++) { - final String threadName = "thread-" + threadId; - Runnable r = () -> { - for (int i = 0; i < iterations; i++) { - try { - if (randomBoolean()) { - String id = "id-" + threadName + "-" + i; - logger.debug("--> {} indexing {}", threadName, id); - indexDoc(primary, "_doc", id, "{\"foo\" : \"" + randomAlphaOfLength(10) + "\"}"); - } - - if (randomBoolean() && i > 10) { - String id = "id-" + threadName + "-" + randomIntBetween(0, i - 1); - logger.debug("--> {}, deleting {}", threadName, id); - deleteDoc(primary, "_doc", id); - } - - if (randomBoolean()) { - logger.debug("--> {} refreshing", threadName); - primary.refresh("forced refresh"); - } - - if (randomBoolean()) { - String searcherName = "searcher-" + threadName + "-" + i; - logger.debug("--> {} acquiring new searcher {}", threadName, searcherName); - // Acquire a new searcher, adding it to the list - searchers.add(primary.acquireSearcher(searcherName)); - } - - if (randomBoolean() && searchers.size() > 1) { - // Close one of the readers at random - synchronized (searchers) { - // re-check because it could have decremented after the check - if (searchers.size() > 1) { - Engine.Searcher searcher = searchers.remove(0); - logger.debug("--> {} closing searcher {}", threadName, searcher.source()); - IOUtils.close(searcher); - } - } - } - } catch (Exception e) { - logger.warn("--> got exception: ", e); - fail("got an exception we didn't expect"); - } - } - - }; - threads.add(new Thread(r, threadName)); - } - threads.stream().forEach(t -> t.start()); - - for (Thread t : threads) { - t.join(); - } - - // We need to wait for all ongoing merges to complete. The reason is that during a merge the - // IndexWriter holds the core cache key open and causes the memory to be registered in the breaker - primary.forceMerge(new ForceMergeRequest().maxNumSegments(1).flush(true)); - - // Close remaining searchers - IOUtils.close(searchers); - primary.refresh("test"); - - SegmentsStats ss = primary.segmentStats(randomBoolean(), randomBoolean()); - CircuitBreaker breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - long segmentMem = ss.getMemoryInBytes(); - long breakerMem = breaker.getUsed(); - logger.info("--> comparing segmentMem: {} - breaker: {} => {}", segmentMem, breakerMem, segmentMem == breakerMem); - assertThat(segmentMem, equalTo(breakerMem)); - - // Close shard - closeShards(primary); - - // Check that the breaker was successfully reset to 0, meaning that all the accounting was correctly applied - breaker = primary.circuitBreakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat(breaker.getUsed(), equalTo(0L)); - } - public void testOnCloseStats() throws IOException { final IndexShard indexShard = newStartedShard(true); diff --git a/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java index a96a37037d016..885c22b017e74 100644 --- a/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java @@ -223,12 +223,9 @@ public void ensureEstimatedStats() { equalTo(0L) ); assertThat( - "Accounting breaker not reset to " - + stats.getIndices().getSegments().getMemoryInBytes() - + " on node: " - + stats.getNode(), + "Accounting breaker not reset to " + stats.getIndices().getSegments().getZeroMemory() + " on node: " + stats.getNode(), stats.getBreaker().getStats(CircuitBreaker.ACCOUNTING).getEstimated(), - equalTo(stats.getIndices().getSegments().getMemoryInBytes()) + equalTo(stats.getIndices().getSegments().getZeroMemory()) ); // ExternalTestCluster does not check the request breaker, // because checking it requires a network request, which in From d60171c83c4aee58d37ec2c4a22b0c5e8f8ca4aa Mon Sep 17 00:00:00 2001 From: Nicholas Walter Knize Date: Tue, 1 Feb 2022 13:30:44 -0600 Subject: [PATCH 2/4] fix test failure Signed-off-by: Nicholas Walter Knize --- .../org/opensearch/index/engine/Segment.java | 6 ++++-- .../index/engine/SegmentsStats.java | 20 ++++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/engine/Segment.java b/server/src/main/java/org/opensearch/index/engine/Segment.java index 5fdff0e52fc5c..2b824c847f75f 100644 --- a/server/src/main/java/org/opensearch/index/engine/Segment.java +++ b/server/src/main/java/org/opensearch/index/engine/Segment.java @@ -71,6 +71,8 @@ public class Segment implements Writeable { public Accountable ramTree = null; public Map attributes; + private static final ByteSizeValue ZERO_BYTE_SIZE_VALUE = new ByteSizeValue(0L); + public Segment(StreamInput in) throws IOException { name = in.readString(); generation = Long.parseLong(name.substring(1), Character.MAX_RADIX); @@ -154,8 +156,8 @@ public String getMergeId() { * @deprecated */ @Deprecated - public long getZeroMemory() { - return 0L; + public ByteSizeValue getZeroMemory() { + return ZERO_BYTE_SIZE_VALUE; } /** diff --git a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java index e1f52ae86d27d..880091c192034 100644 --- a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java +++ b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java @@ -53,6 +53,8 @@ public class SegmentsStats implements Writeable, ToXContentFragment { private long bitsetMemoryInBytes; private ImmutableOpenMap fileSizes = ImmutableOpenMap.of(); + private static final ByteSizeValue ZERO_BYTE_SIZE_VALUE = new ByteSizeValue(0L); + /* * A map to provide a best-effort approach describing Lucene index files. * @@ -214,13 +216,13 @@ public long getMaxUnsafeAutoIdTimestamp() { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(Fields.SEGMENTS); builder.field(Fields.COUNT, count); - builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, 0L); - builder.humanReadableField(Fields.TERMS_MEMORY_IN_BYTES, Fields.TERMS_MEMORY, 0L); - builder.humanReadableField(Fields.STORED_FIELDS_MEMORY_IN_BYTES, Fields.STORED_FIELDS_MEMORY, 0L); - builder.humanReadableField(Fields.TERM_VECTORS_MEMORY_IN_BYTES, Fields.TERM_VECTORS_MEMORY, 0L); - builder.humanReadableField(Fields.NORMS_MEMORY_IN_BYTES, Fields.NORMS_MEMORY, 0L); - builder.humanReadableField(Fields.POINTS_MEMORY_IN_BYTES, Fields.POINTS_MEMORY, 0L); - builder.humanReadableField(Fields.DOC_VALUES_MEMORY_IN_BYTES, Fields.DOC_VALUES_MEMORY, 0L); + builder.humanReadableField(Fields.MEMORY_IN_BYTES, Fields.MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.TERMS_MEMORY_IN_BYTES, Fields.TERMS_MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.STORED_FIELDS_MEMORY_IN_BYTES, Fields.STORED_FIELDS_MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.TERM_VECTORS_MEMORY_IN_BYTES, Fields.TERM_VECTORS_MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.NORMS_MEMORY_IN_BYTES, Fields.NORMS_MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.POINTS_MEMORY_IN_BYTES, Fields.POINTS_MEMORY, ZERO_BYTE_SIZE_VALUE); + builder.humanReadableField(Fields.DOC_VALUES_MEMORY_IN_BYTES, Fields.DOC_VALUES_MEMORY, ZERO_BYTE_SIZE_VALUE); builder.humanReadableField(Fields.INDEX_WRITER_MEMORY_IN_BYTES, Fields.INDEX_WRITER_MEMORY, getIndexWriterMemory()); builder.humanReadableField(Fields.VERSION_MAP_MEMORY_IN_BYTES, Fields.VERSION_MAP_MEMORY, getVersionMapMemory()); builder.humanReadableField(Fields.FIXED_BIT_SET_MEMORY_IN_BYTES, Fields.FIXED_BIT_SET, getBitsetMemory()); @@ -303,7 +305,7 @@ public void clearFileSizes() { * @deprecated */ @Deprecated - public Long getZeroMemory() { - return 0L; + public ByteSizeValue getZeroMemory() { + return ZERO_BYTE_SIZE_VALUE; } } From 4bf5a5175b4c75a602f3f0cdc52ef7217d431d0e Mon Sep 17 00:00:00 2001 From: Nicholas Walter Knize Date: Tue, 1 Feb 2022 14:05:24 -0600 Subject: [PATCH 3/4] remove unnecessary assert from ExternalTestCluster Signed-off-by: Nicholas Walter Knize --- .../main/java/org/opensearch/test/ExternalTestCluster.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java index 885c22b017e74..805e578a8e8db 100644 --- a/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/ExternalTestCluster.java @@ -222,11 +222,6 @@ public void ensureEstimatedStats() { stats.getBreaker().getStats(CircuitBreaker.FIELDDATA).getEstimated(), equalTo(0L) ); - assertThat( - "Accounting breaker not reset to " + stats.getIndices().getSegments().getZeroMemory() + " on node: " + stats.getNode(), - stats.getBreaker().getStats(CircuitBreaker.ACCOUNTING).getEstimated(), - equalTo(stats.getIndices().getSegments().getZeroMemory()) - ); // ExternalTestCluster does not check the request breaker, // because checking it requires a network request, which in // turn increments the breaker, making it non-0 From 693ed1c4f1178cb071737e87b44f5bf3985f091f Mon Sep 17 00:00:00 2001 From: Nicholas Walter Knize Date: Tue, 1 Feb 2022 16:02:19 -0600 Subject: [PATCH 4/4] fix RestSegmentsAction to pass long not ByteSizeValue Signed-off-by: Nicholas Walter Knize --- .../java/org/opensearch/rest/action/cat/RestSegmentsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java b/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java index f2e0e49fe97b5..8d9d1937bdf56 100644 --- a/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java +++ b/server/src/main/java/org/opensearch/rest/action/cat/RestSegmentsAction.java @@ -155,7 +155,7 @@ private Table buildTable(final RestRequest request, ClusterStateResponse state, table.addCell(segment.getNumDocs()); table.addCell(segment.getDeletedDocs()); table.addCell(segment.getSize()); - table.addCell(segment.getZeroMemory()); + table.addCell(0L); table.addCell(segment.isCommitted()); table.addCell(segment.isSearch()); table.addCell(segment.getVersion());