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

Should search preference _primary take primary relocation target into account? #26335

Closed
ywelsch opened this issue Aug 23, 2017 · 22 comments
Closed
Assignees
Labels
discuss good first issue low hanging fruit :Search/Search Search-related issues that do not fall into other categories

Comments

@ywelsch
Copy link
Contributor

ywelsch commented Aug 23, 2017

When using the search preference _primary, and executing a search on a cluster where a concerned primary is about to complete relocation, it's possible that the search for that primary will fail, as it will hit the primary relocation source just when the relocation is completed and the source shard is shut down. My question is:

Should the search preference _primary take the primary relocation target into account?
Put differently, should OperationRouting.preferenceActiveShardIterator(...) return the primary relocation target as backup when specifying _primary preference?

Note that the default search preference always takes relocation targets into account (and puts them last together with the "regular" initializing shards).

Related question:

Should the search preference _replica also take replica relocation targets into account?

More background to this:

There was a test failure of SearchWhileCreatingIndexIT which illustrates the issue:

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+g1gc/4012/consoleFull

@ywelsch ywelsch added :Search/Search Search-related issues that do not fall into other categories discuss labels Aug 23, 2017
@bleskes
Copy link
Contributor

bleskes commented Aug 23, 2017

I'm struggling to find a use case for _primary and _replica. Should we deprecate and remove them?

@dakrone
Copy link
Member

dakrone commented Aug 23, 2017

If I recall correctly, _replica was originally added back in the day for shadow replicas (see: #12244). Which have been removed, so +1 on removing _replica and _replica_first.

_primary was useful back in the days of asynchronous replication, since your document may not have reached the replicas yet. We don't allow it any more though, so it's probably not strictly necessary.

However, having _primary and _replica can be nice for debugging things, for instance, if you suspect that a copy of the data is somehow misbehaving, or you want to compare an analyzer on one node with a (perhaps misconfigured) analyzer on a second node, it can be very helpful to be able to use _primary and _replica to route the request to the desired node, so overall I'm probably +0.5 on keeping them.

@bleskes
Copy link
Contributor

bleskes commented Aug 24, 2017

@dakrone thanks for filling in the context and history. I should have done it in the first place.

However, having _primary and _replica can be nice for debugging things

I think this use case is better served by the _prefer_nodes and _only_nodes options. It works for primaries (with some extra effort of figuring out where the primary is) and is actually needed for replicas when people run with >1 replicas.

@dakrone
Copy link
Member

dakrone commented Aug 24, 2017

I think this use case is better served by the _prefer_nodes and _only_nodes options.

Those work also, so +1 to removing the others if no one else objects. Perhaps the title of this issues should be changed to reflect the discussion?

@bleskes bleskes added help wanted adoptme good first issue low hanging fruit and removed discuss labels Aug 25, 2017
@liketic
Copy link

liketic commented Sep 13, 2017

From what I understand, we only need to keep the two preference _prefer_nodes and _only_nodes and remove all others? Thanks!

@dakrone
Copy link
Member

dakrone commented Sep 13, 2017

@liketic there are others that should still be kept, I believe the consensus was just to remove _primary and _replica

@bleskes
Copy link
Contributor

bleskes commented Sep 13, 2017

@dakrone Do we have a valid use case for (_primary|_replica)_first?

@dakrone
Copy link
Member

dakrone commented Sep 13, 2017

@bleskes other than them being nicer than having to determine the nodes themselves? I'm not sure. The only thing I could think would be a very script-heavy update where you wanted to use _replica so that you avoided load on the primaries that were doing the fetch-run-script-and-update.

@bleskes
Copy link
Contributor

bleskes commented Sep 13, 2017

@bleskes other than them being nicer than having to determine the nodes themselves?

The question is why would you like to do that? I would op to remove those. It just confuses people to think that primaries are special (and we should remove cases where they are..)

@dakrone
Copy link
Member

dakrone commented Sep 13, 2017

The question is why would you like to do that?

That (the update scenario) was the only scenario I could think of

I would op to remove those.

Sure, if you think the scenario I mentioned isn't valid

It just confuses people to think that primaries are special (and we should remove cases where they are..)

I'm not sure if we're going to be able to move away from update scripts running on the primary shard? At least definitely not on the short term

@liketic
Copy link

liketic commented Sep 14, 2017

So it's OK to remove _primary and _replica now? Thanks. 😄

@bleskes
Copy link
Contributor

bleskes commented Sep 14, 2017

@dakrone OK. I'll add a discuss label to this so it will be picked up in fix it friday and we make a final decision. I tend to say remove them all (we can fix the update issue by making it read from any replica, but I agree that's not happening soon)

@liketic can you hold off for a few days? I appreciate the willing to contribute.

@bleskes bleskes added :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure discuss and removed :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure labels Sep 14, 2017
@dnhatn dnhatn self-assigned this Sep 25, 2017
@dnhatn dnhatn removed the help wanted adoptme label Sep 25, 2017
@dnhatn
Copy link
Member

dnhatn commented Sep 25, 2017

@jasontedor, I just want to make sure that I understand the requirement correctly. We will deprecate the preference "_primary*" and "_replica*" in 6.x and remove them in 7.x. Thank you.

@jasontedor
Copy link
Member

@dnhatn Yes.

@dnhatn
Copy link
Member

dnhatn commented Sep 26, 2017

@jasontedor I am not sure if the recommendation to use preference=_primary for read-then-update is still valid. We document it at https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-index_.html#index-versioning

@jasontedor
Copy link
Member

@dnhatn Not reading from the primary is okay in the case of optimistic concurrency control; if the read is from a stale value, the dependent write will be rejected anyway and the read will simply have to be retried. This is okay for a workflow using optimistic concurrency control. Therefore, I think we can simply remove this recommendation from the documentation.

dnhatn added a commit to dnhatn/elasticsearch that referenced this issue Sep 26, 2017
The shard preference `_primary`, `_replica` and its variants were useful
for the asynchronous replication. However, with the current impl, they
are no longer useful and should be removed.

Closes elastic#26335
@dnhatn dnhatn added the review label Sep 26, 2017
dnhatn added a commit to dnhatn/elasticsearch that referenced this issue Sep 26, 2017
dnhatn added a commit that referenced this issue Oct 8, 2017
The shard preference _primary, _replica and its variants were useful
for the asynchronous replication. However, with the current impl, they
are no longer useful and should be removed.

Closes #26335
dnhatn added a commit that referenced this issue Oct 9, 2017
Tells v6 users that we are going to remove these options in v7.

Relates #26335
@aewhite
Copy link

aewhite commented Feb 6, 2018

Sorry, late to the conversation on this but we use _primary as a cache hitting optimization. Unless something has changed, ES can hit non-primary shards to answer search requests. By preferring primaries only then filter caches:

  • Warm up faster
  • Have a higher cache hit rate
  • Are more "dense" (ie, a node only has to maintain primary caches)

@bleskes
Copy link
Contributor

bleskes commented Feb 7, 2018

@aewhite you can better use a random string which will stick to a random copy (see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-preference.html ). ES assumes primaries do just as much work as replicas when balancing the cluster. As noted about the _primary preference is dangerous. Anything that sticks to the primary may cause hot spots. I presume you have enough resources since you don't rely on your replicas at all, but still.

@paullovessearch
Copy link

paullovessearch commented Apr 27, 2018

I too am frustrated that this is being removed. We have lots of situations with very large indexes and very low query volume. When only one query is likely to be executing at a time, it is preferred to use the primary shards so that the index files can be cached into OS disk cache. Having the queries go back and forth between primary and replica simply means doubling the amount of RAM required for the same performance. The "random string" doesn't help at all for this use case and I'm not even sure why it was mentioned. We have low query volume (like, 1 QPS) so hot spots are not an issue. And we want the system to automatically fail over to the replica when the primary machine goes down and suffer some degraded performance until the machine is restored.

Large index + low query volume is quite a common use case in our world, and it seems that you're forcing us to double our OS disk cache RAM requirement. Do you have recommendations for reducing hardware cost for this scenario?

@DaveCTurner
Copy link
Contributor

... it is preferred to use the primary shards so that the index files can be cached into OS disk cache

It makes sense that you want to route repeated queries to the same set of shard copies each time to make best use of disk cache, but there's no need for those copies to be primaries.

Having the queries go back and forth between primary and replica simply means doubling the amount of RAM required for the same performance.

I think you're misunderstanding what happens when using GET /_search?preference=xyzabc123. The query is routed to one copy of each shard, which may or may not be the primary, but there's no communication between primary and replica. In the steady state, the copy chosen is a pure function of the xyzabc123 string, so repeated queries will hit the same copy each time.

Indeed, the random string should give you more even node usage than _primary after a node restart, because the restarted node will hold no primaries, possibly for a long time, but the "random string" preference will start to make use of the new node right away.

@paullovessearch
Copy link

I see. So you're saying to use the same preference value for all queries, and not a random unique one for every query. Okay - so that makes sense.

And I presume that if the normal shard targeted by preference=X goes down, then the replica will be used? If so, then I completely agree that this is a fine solution. Thanks!

@DaveCTurner
Copy link
Contributor

I presume that if the normal shard targeted by preference=X goes down, then the replica will be used?

Yes. ?preference=X really chooses a preferred order of all the available shard copies to try in a search, and so will fail-over sensibly if a node goes down.

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this issue Jul 16, 2018
Today it is unclear what guarantees are offered by the search preference
feature, and we claim a guarantee that is stronger than what we really offer:

> A custom value will be used to guarantee that the same shards will be used
> for the same custom value.

This commit clarifies this documentation and explains more clearly why
`_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`.

Relates elastic#31929 elastic#26335 elastic#26791.
DaveCTurner added a commit that referenced this issue Jul 18, 2018
Today it is unclear what guarantees are offered by the search preference
feature, and we claim a guarantee that is stronger than what we really offer:

> A custom value will be used to guarantee that the same shards will be used
> for the same custom value.

This commit clarifies this documentation and explains more clearly why
`_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`.

Relates #31929 #26335 #26791.
DaveCTurner added a commit that referenced this issue Jul 18, 2018
Today it is unclear what guarantees are offered by the search preference
feature, and we claim a guarantee that is stronger than what we really offer:

> A custom value will be used to guarantee that the same shards will be used
> for the same custom value.

This commit clarifies this documentation and explains more clearly why
`_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`.

Relates #31929 #26335 #26791.
DaveCTurner added a commit that referenced this issue Jul 18, 2018
Today it is unclear what guarantees are offered by the search preference
feature, and we claim a guarantee that is stronger than what we really offer:

> A custom value will be used to guarantee that the same shards will be used
> for the same custom value.

This commit clarifies this documentation and explains more clearly why
`_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`.

Relates #31929 #26335 #26791.
DaveCTurner added a commit that referenced this issue Jul 18, 2018
Today it is unclear what guarantees are offered by the search preference
feature, and we claim a guarantee that is stronger than what we really offer:

> A custom value will be used to guarantee that the same shards will be used
> for the same custom value.

This commit clarifies this documentation and explains more clearly why
`_primary`, `_replica`, etc. are deprecated in `6.x` and removed in `master`.

Relates #31929 #26335 #26791.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss good first issue low hanging fruit :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

No branches or pull requests

9 participants