Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Return true for can_match on idle search shards #55428

Merged
merged 8 commits into from
Apr 26, 2020
Merged

Return true for can_match on idle search shards #55428

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2343f40
Can_match requests should wait for search active
dnhatn Apr 18, 2020
d30c219
Merge branch 'master' into can_match_wait_for_refresh
dnhatn Apr 18, 2020
e91b718
Merge branch 'master' into can_match_wait_for_refresh
dnhatn Apr 20, 2020
3b1dde9
skip can_match
dnhatn Apr 20, 2020
09c7e66
unused imports
dnhatn Apr 20, 2020
0e7f976
pending refresh
dnhatn Apr 20, 2020
7d4bc41
Merge branch 'master' into can_match_wait_for_refresh
dnhatn Apr 21, 2020
80f492f
minAndMax -> estimatedMinAndMax
dnhatn Apr 21, 2020
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
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/index/shard/IndexShard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3173,7 +3173,7 @@ && isSearchIdle()
/**
* Returns true if this shards is search idle
*/
final boolean isSearchIdle() {
public final boolean isSearchIdle() {
return (threadPool.relativeTimeInMillis() - lastSearcherAccess.get()) >= indexSettings.getSearchIdleAfter().getMillis();
}

Expand Down
3 changes: 3 additions & 0 deletions server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,9 @@ public CanMatchResponse canMatch(ShardSearchRequest request) throws IOException
assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType();
IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex());
IndexShard indexShard = indexService.getShard(request.shardId().getId());
if (indexShard.isSearchIdle()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to check whether there are changes that have not been refreshed? Otherwise this change will have a terrible effiect on the warm tier by returning can_match=true up to once per second even though no changes are being made to indices? Or are we already protected against this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I pushed 0e7f976.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can still extract the min/max values of the primary sort since we don't need the values to be accurate and use it only to order the shards execution. If indices are in the warm tier they should contain some recent data that we'd like to visit first if the query is sorted by timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds potentially dangerous to me. I wouldn't think of the special case of search-idle indices if someone raised an idea that would leverage min/max values and rely on the fact that the max value to be an upper bound and the min value to be a lower bound?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have this guarantee today either, the reader of the query phase can be different from the one used in the can match phase. I can update the comments to make it more clear in the code that we don't create a point in time reader during the can match phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the can_match phase might get a different reader from the query phase, but I'm less concerned by this since the reader we get in the can_match phase would also be a legal reader to run the query phase on, which isn't the case here? I don't feel strongly about this, ok to return the min/max values and +1 to document this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I also thought about this because having these guarantees would mean that we can filter shards on the coord node based on these values. We don't do this today because of search-idle shards so we delay the skipping when verifying the values locally during the query phase.
The other option that was proposed by Nhat in this pr was to perform the refresh of search-idle shards during the can_match phase. It has the advantage of making all min/max values accurate for the search request but could make the phase much slower.

I don't feel strongly about this, ok to return the min/max values and +1 to document this.

++, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Adrien and Jim for the discussion here. I pushed 80f492f.

return new CanMatchResponse(true, null);
}
// we don't want to use the reader wrapper since it could run costly operations
// and we can afford false positives.
try (Engine.Searcher searcher = indexShard.acquireCanMatchSearcher()) {
Expand Down
35 changes: 35 additions & 0 deletions server/src/test/java/org/elasticsearch/action/search/TransportSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,18 @@

import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.test.ESIntegTestCase;

import java.util.Collections;

import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;

public class TransportSearchIT extends ESIntegTestCase {

Expand Down Expand Up @@ -69,4 +75,33 @@ public void testShardCountLimit() throws Exception {
TransportSearchAction.SHARD_COUNT_LIMIT_SETTING.getKey(), null)));
}
}

public void testSearchIdle() throws Exception {
int numOfReplicas = randomIntBetween(0, 1);
internalCluster().ensureAtLeastNumDataNodes(numOfReplicas + 1);
final Settings.Builder settings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, randomIntBetween(1, 5))
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numOfReplicas)
.put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), TimeValue.timeValueMillis(randomIntBetween(50, 500)));
assertAcked(prepareCreate("test").setSettings(settings)
.setMapping("{\"properties\":{\"created_date\":{\"type\": \"date\", \"format\": \"yyyy-MM-dd\"}}}"));
ensureGreen("test");
assertBusy(() -> {
for (String node : internalCluster().nodesInclude("test")) {
final IndicesService indicesService = internalCluster().getInstance(IndicesService.class, node);
for (IndexShard indexShard : indicesService.indexServiceSafe(resolveIndex("test"))) {
assertTrue(indexShard.isSearchIdle());
}
}
});
client().prepareIndex("test").setId("1").setSource("created_date", "2020-01-01").get();
client().prepareIndex("test").setId("2").setSource("created_date", "2020-01-02").get();
client().prepareIndex("test").setId("3").setSource("created_date", "2020-01-03").get();
assertBusy(() -> {
SearchResponse resp = client().prepareSearch("test")
.setQuery(new RangeQueryBuilder("created_date").gte("2020-01-02").lte("2020-01-03"))
.setPreFilterShardSize(randomIntBetween(1, 3)).get();
assertThat(resp.getHits().getTotalHits().value, equalTo(2L));
});
}
}