Skip to content

Return cached segments stats if include_unloaded_segments is true #39698

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@
"type": "boolean",
"description": "If set to true segment stats will include stats for segments that are not currently loaded into memory",
"default": false
},
"expand_wildcards": {
"type" : "enum",
"options" : ["open","closed","none","all"],
"default" : "open",
"description" : "Whether to expand wildcard expression to concrete indices that are open, closed or both."
},
"forbid_closed_indices": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we introducing this parameter only for BWC of behavior in 7.x or do you think this could still be useful in 8.0? In 8.0 I would expect this parameter to be false by default, and I'm not sure why I would ever explicitly set it to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ywelsch I must have missed something, the tests failed if I'd not have that in there. Maybe I ran tests before the relevant change was introduced?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a misunderstanding. What I meant by "In 8.0 I would expect this parameter to be false by default" is that I think we should make it default to false in 8.0, so that if you include closed indices in your indices stats request, that the request does not fail by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ywelsch I would like to merge as is and then go and change the defaults for stats in a separate pr. ok with you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let's do it like that

"type": "boolean",
"description": "If set to false stats will also collected from closed indices if explicitly specified or if expand_wildcards expands to closed indices",
"default": true
}
}
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
---
setup:
- do:
indices.create:
index: test
body:
settings:
number_of_shards: 1
number_of_replicas: 0

- do:
cluster.health:
wait_for_no_initializing_shards: true

---
"Segment Stats":

- skip:
version: " - 7.99.99"
reason: forbid_closed_indices is not supported in ealier version

- do:
indices.stats:
metric: [ segments ]
- set: { indices.test.primaries.segments.count: num_segments }

- do:
index:
index: test
id: 1
body: { "foo": "bar" }

- do:
indices.flush:
index: test

- do:
indices.stats:
metric: [ segments ]
- gt: { indices.test.primaries.segments.count: $num_segments }
- set: { indices.test.primaries.segments.count: num_segments_after_flush }

- do:
indices.close:
index: test

- do:
indices.stats:
metric: segments
expand_wildcards: closed
forbid_closed_indices: false

- match: { indices.test.primaries.segments.count: 0 }

- do:
indices.stats:
metric: segments
include_unloaded_segments: true
expand_wildcards: closed
forbid_closed_indices: false

- match: { indices.test.primaries.segments.count: $num_segments_after_flush }
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ public IndicesStatsRequest includeSegmentFileSizes(boolean includeSegmentFileSiz
return this;
}

public IndicesStatsRequest includeUnloadedSegments(boolean includeUnloadedSegments) {
flags.includeUnloadedSegments(includeUnloadedSegments);
return this;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@
import org.apache.lucene.index.IndexCommit;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.SegmentReader;
import org.apache.lucene.store.Directory;
import org.elasticsearch.common.lucene.Lucene;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.List;
import java.util.function.Function;

Expand All @@ -36,8 +40,20 @@
*/
public final class NoOpEngine extends ReadOnlyEngine {

private final SegmentsStats stats;

public NoOpEngine(EngineConfig config) {
super(config, null, null, true, Function.identity());
this.stats = new SegmentsStats();
Directory directory = store.directory();
try (DirectoryReader reader = DirectoryReader.open(directory)) {
for (LeafReaderContext ctx : reader.getContext().leaves()) {
SegmentReader segmentReader = Lucene.segmentReader(ctx.reader());
fillSegmentStats(segmentReader, true, stats);
}
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

@Override
Expand All @@ -47,17 +63,17 @@ protected DirectoryReader open(final IndexCommit commit) throws IOException {
final IndexCommit indexCommit = indexCommits.get(indexCommits.size() - 1);
return new DirectoryReader(directory, new LeafReader[0]) {
@Override
protected DirectoryReader doOpenIfChanged() throws IOException {
protected DirectoryReader doOpenIfChanged() {
return null;
}

@Override
protected DirectoryReader doOpenIfChanged(IndexCommit commit) throws IOException {
protected DirectoryReader doOpenIfChanged(IndexCommit commit) {
return null;
}

@Override
protected DirectoryReader doOpenIfChanged(IndexWriter writer, boolean applyAllDeletes) throws IOException {
protected DirectoryReader doOpenIfChanged(IndexWriter writer, boolean applyAllDeletes) {
return null;
}

Expand All @@ -67,17 +83,17 @@ public long getVersion() {
}

@Override
public boolean isCurrent() throws IOException {
public boolean isCurrent() {
return true;
}

@Override
public IndexCommit getIndexCommit() throws IOException {
public IndexCommit getIndexCommit() {
return indexCommit;
}

@Override
protected void doClose() throws IOException {
protected void doClose() {
}

@Override
Expand All @@ -86,4 +102,18 @@ public CacheHelper getReaderCacheHelper() {
}
};
}

@Override
public SegmentsStats segmentsStats(boolean includeSegmentFileSizes, boolean includeUnloadedSegments) {
if (includeUnloadedSegments) {
final SegmentsStats stats = new SegmentsStats();
stats.add(this.stats);
if (includeSegmentFileSizes == false) {
stats.clearFileSizes();
}
return stats;
} else {
return super.segmentsStats(includeSegmentFileSizes, includeUnloadedSegments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling this will fail on a NoopEngine?
Should we instead return an empty stats object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm there is a test for this here that shows that it's not failing but returning an empty object.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ public String getName() {
@Override
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
IndicesStatsRequest indicesStatsRequest = new IndicesStatsRequest();
indicesStatsRequest.indicesOptions(IndicesOptions.fromRequest(request, indicesStatsRequest.indicesOptions()));
boolean forbidClosedIndices = request.paramAsBoolean("forbid_closed_indices", true);
IndicesOptions defaultIndicesOption = forbidClosedIndices ? indicesStatsRequest.indicesOptions()
: IndicesOptions.strictExpandOpen();
assert indicesStatsRequest.indicesOptions() == IndicesOptions.strictExpandOpenAndForbidClosed() : "IndicesStats default indices " +
"options changed";
indicesStatsRequest.indicesOptions(IndicesOptions.fromRequest(request, defaultIndicesOption));
indicesStatsRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
indicesStatsRequest.types(Strings.splitStringByCommaToArray(request.param("types")));

Expand Down Expand Up @@ -121,7 +126,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC

if (indicesStatsRequest.segments()) {
indicesStatsRequest.includeSegmentFileSizes(request.paramAsBoolean("include_segment_file_sizes", false));
indicesStatsRequest.includeSegmentFileSizes(request.paramAsBoolean("include_unloaded_segments", false));
indicesStatsRequest.includeUnloadedSegments(request.paramAsBoolean("include_unloaded_segments", false));
}

return channel -> client.admin().indices().stats(indicesStatsRequest, new RestToXContentListener<>(channel));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public void processResponse(final ClusterHealthResponse clusterHealthResponse) {
indicesStatsRequest.indices(indices);
indicesStatsRequest.indicesOptions(strictExpandIndicesOptions);
indicesStatsRequest.all();
indicesStatsRequest.includeSegmentFileSizes(request.paramAsBoolean("include_unloaded_segments", false));
indicesStatsRequest.includeUnloadedSegments(request.paramAsBoolean("include_unloaded_segments", false));

client.admin().indices().stats(indicesStatsRequest, new RestResponseListener<IndicesStatsResponse>(channel) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void testNoopAfterRegularEngine() throws IOException {
noOpEngine.close();
}

public void testNoOpEngineDocStats() throws Exception {
public void testNoOpEngineStats() throws Exception {
IOUtils.close(engine, store);
final AtomicLong globalCheckpoint = new AtomicLong(SequenceNumbers.NO_OPS_PERFORMED);
try (Store store = createStore()) {
Expand Down Expand Up @@ -131,15 +131,25 @@ public void testNoOpEngineDocStats() throws Exception {
}

final DocsStats expectedDocStats;
boolean includeFileSize = randomBoolean();
final SegmentsStats expectedSegmentStats;
try (InternalEngine engine = createEngine(config)) {
expectedDocStats = engine.docStats();
expectedSegmentStats = engine.segmentsStats(includeFileSize, true);
}

try (NoOpEngine noOpEngine = new NoOpEngine(config)) {
assertEquals(expectedDocStats.getCount(), noOpEngine.docStats().getCount());
assertEquals(expectedDocStats.getDeleted(), noOpEngine.docStats().getDeleted());
assertEquals(expectedDocStats.getTotalSizeInBytes(), noOpEngine.docStats().getTotalSizeInBytes());
assertEquals(expectedDocStats.getAverageSizeInBytes(), noOpEngine.docStats().getAverageSizeInBytes());
assertEquals(expectedSegmentStats.getCount(), noOpEngine.segmentsStats(includeFileSize, true).getCount());
assertEquals(expectedSegmentStats.getMemoryInBytes(), noOpEngine.segmentsStats(includeFileSize, true).getMemoryInBytes());
assertEquals(expectedSegmentStats.getFileSizes().size(),
noOpEngine.segmentsStats(includeFileSize, true).getFileSizes().size());

assertEquals(0, noOpEngine.segmentsStats(includeFileSize, false).getFileSizes().size());
assertEquals(0, noOpEngine.segmentsStats(includeFileSize, false).getMemoryInBytes());
} catch (AssertionError e) {
logger.error(config.getMergePolicy());
throw e;
Expand Down