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

Retries are dangerous for scrolled search queries #610

Closed
jordanlibrande opened this issue Sep 20, 2017 · 3 comments
Closed

Retries are dangerous for scrolled search queries #610

jordanlibrande opened this issue Sep 20, 2017 · 3 comments
Assignees

Comments

@jordanlibrande
Copy link

Retrying a scrolled search query that started executing, but failed on timeout/connection issue will lead to that page being skipped, thus leading to incomplete results across the entire set of queries.

I'm not sure exactly what the library improvement would be, but I'm hoping to improve the library somehow so other people don't get hit with this unexpected behavior.

In my use case, I would always prefer to error out, rather than skip a page of results. I'm planning to work around this on my end by creating a custom retrier for scrolled search, this retrier will retry on "node not available" and some similar errors, but will not retry on timeout errors. Then I'll create two elastic clients, one for normal search with a normal retrier, and one for scrolled search with the custom scrolled retrier.

Eventually this core issue of scrolled search being bad might be fixed in a later ES version via something like elastic/elasticsearch#26472

Possible approaches here could include:

  • better package documentation for retriers about the danger of retrying scrolled search
  • another new built-in retrier that only retries on "safe" errors
  • client stores separate retrier objects for use in normal and scrolled searches
@olivere olivere self-assigned this Sep 21, 2017
@olivere
Copy link
Owner

olivere commented Sep 21, 2017

Thanks for bringing this up. I will tinker with this as well. Maybe we find some good solutions that everyone is happy with.

@olivere
Copy link
Owner

olivere commented Oct 16, 2017

I think what could work is to

a) use a special built-in retrier for ScrollService that only retries on "safe" errors (skipping the default retrier on the Client),
b) allow users to configure a custom retrier in ScrollService, replacing the default one in a), and
c) make Client.PerformRequest accept a Retrier (using the retrier on the Client by default).

ScrollService will pass its custom/configured Retrier to Client.PerformRequest, while other services will use the default.

olivere added a commit that referenced this issue Jan 8, 2018
This commit extends the `PerformRequestOptions` to pass a custom
`Retrier` per request. This is enabled for the Scroll and Bulk API for
now.

See #666 and #610
olivere added a commit that referenced this issue Jan 8, 2018
This commit backports the `PerformRequestOptions` structure from v6 and
introduces `PerformRequestWithOptions`, which allows us to extend the
list of parameters passed when executing a HTTP request.

This change allows us to now also pass a custom `Retrier` per request,
which we allow for Scroll API and Bulk API via `Retrier(...)` on the
service respectively.

Backported from d2219c2.

See also #666 and #610
@olivere
Copy link
Owner

olivere commented Jan 8, 2018

I just released 5.0.60 and 6.1.1 which both allow to set a per-request Retrier (with configurable Backoff). For now, only Bulk API and Scroll API make use of that. Use Retrier(...) on the BulkService or ScrollService to pass your own retrier for the given service.

@olivere olivere closed this as completed Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants