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.

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

Revert fast refresh using search shards #115019

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

kingherc
Copy link
Contributor

@kingherc kingherc commented Oct 17, 2024

As this induces ES-8275 and makes fleet time outs for some APIs.

Reverts #113478 .

Relates ES-9573

As this induces ES-8275 and makes fleet time outs for some APIs.

Relates ES-9573
@kingherc kingherc self-assigned this Oct 17, 2024
@elasticsearchmachine elasticsearchmachine added v9.0.0 serverless-linked Added by automation, don't add manually labels Oct 17, 2024
@kingherc kingherc added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >non-issue labels Oct 17, 2024
@@ -103,7 +105,10 @@ static boolean shouldLoadRandomAccessFiltersEagerly(IndexSettings settings) {
boolean loadFiltersEagerlySetting = settings.getValue(INDEX_LOAD_RANDOM_ACCESS_FILTERS_EAGERLY_SETTING);
boolean isStateless = DiscoveryNode.isStateless(settings.getNodeSettings());
if (isStateless) {
return loadFiltersEagerlySetting && DiscoveryNode.hasRole(settings.getNodeSettings(), DiscoveryNodeRole.SEARCH_ROLE);
return loadFiltersEagerlySetting
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure whether to revert this part to its original state, as it was wrong. This has the discussion. Basically the cache should be loaded whenever the shard can be searched. But welcome feedback.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@kingherc kingherc marked this pull request as ready for review October 17, 2024 15:13
@kingherc kingherc requested a review from tlrx October 17, 2024 15:13
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -126,10 +126,11 @@ protected void asyncShardOperation(GetRequest request, ShardId shardId, ActionLi
IndexService indexService = indicesService.indexServiceSafe(shardId.getIndex());
IndexShard indexShard = indexService.getShard(shardId.id());
if (indexShard.routingEntry().isPromotableToPrimary() == false) {
assert indexShard.indexSettings().isFastRefresh() == false
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea how serverless upgrade tests are working and we'll see what CI says, but I suppose this can trip if a request is wrongly redirected in a mixed cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, lets not risk it. I'll comment the assertion and add a TODO.

@@ -103,7 +105,10 @@ static boolean shouldLoadRandomAccessFiltersEagerly(IndexSettings settings) {
boolean loadFiltersEagerlySetting = settings.getValue(INDEX_LOAD_RANDOM_ACCESS_FILTERS_EAGERLY_SETTING);
boolean isStateless = DiscoveryNode.isStateless(settings.getNodeSettings());
if (isStateless) {
return loadFiltersEagerlySetting && DiscoveryNode.hasRole(settings.getNodeSettings(), DiscoveryNodeRole.SEARCH_ROLE);
return loadFiltersEagerlySetting
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@kingherc kingherc enabled auto-merge (squash) October 17, 2024 16:51
@kingherc kingherc merged commit d3fcead into elastic:main Oct 17, 2024
16 checks passed
lkts pushed a commit to lkts/elasticsearch that referenced this pull request Oct 18, 2024
As this induces ES-8275 and makes fleet time outs for some APIs.

Relates ES-9573
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024
As this induces ES-8275 and makes fleet time outs for some APIs.

Relates ES-9573
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
As this induces ES-8275 and makes fleet time outs for some APIs.

Relates ES-9573
kingherc added a commit to kingherc/elasticsearch that referenced this pull request Nov 12, 2024
The changes of PR elastic#115019 were reverted because it induced ES-8275. Now
that the ticket is done, this PR re-introduces the reverted changes.

Fast refresh indices should now behave like non fast refresh indices in
how they execute (m)gets and searches. I.e., they should use the search
shards.

For BWC, we define a new transport version. We expect search shards to
be upgraded first, before promotable shards. Until the cluster is fully
upgraded, the promotable shards (whether upgraded or not) will still
receive and execute gets/searches locally.

Relates ES-9573
kingherc added a commit that referenced this pull request Nov 19, 2024
The changes of PR #115019 were reverted because it induced ES-8275. Now
that the ticket is done, this PR re-introduces the reverted changes.

Fast refresh indices should now behave like non fast refresh indices in
how they execute (m)gets and searches. I.e., they should use the search
shards.

For BWC, we define a new transport version. We expect search shards to
be upgraded first, before promotable shards. Until the cluster is fully
upgraded, the promotable shards (whether upgraded or not) will still
receive and execute gets/searches locally.

Relates ES-9573
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 20, 2024
The changes of PR elastic#115019 were reverted because it induced ES-8275. Now
that the ticket is done, this PR re-introduces the reverted changes.

Fast refresh indices should now behave like non fast refresh indices in
how they execute (m)gets and searches. I.e., they should use the search
shards.

For BWC, we define a new transport version. We expect search shards to
be upgraded first, before promotable shards. Until the cluster is fully
upgraded, the promotable shards (whether upgraded or not) will still
receive and execute gets/searches locally.

Relates ES-9573
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
The changes of PR elastic#115019 were reverted because it induced ES-8275. Now
that the ticket is done, this PR re-introduces the reverted changes.

Fast refresh indices should now behave like non fast refresh indices in
how they execute (m)gets and searches. I.e., they should use the search
shards.

For BWC, we define a new transport version. We expect search shards to
be upgraded first, before promotable shards. Until the cluster is fully
upgraded, the promotable shards (whether upgraded or not) will still
receive and execute gets/searches locally.

Relates ES-9573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue serverless-linked Added by automation, don't add manually Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants