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

Fix reindex from remote clearing scroll #22525

Merged
merged 1 commit into from Jan 10, 2017

Conversation

Projects
None yet
4 participants
@nik9000
Copy link
Contributor

commented Jan 10, 2017

Reindex-from-remote had a race when it tried to clear the scroll. It
first starts the request to clear the scroll and then submits a task
to the generic threadpool to shutdown the client. These two things
race and, in my experience, closing the scroll generally loses. That
means that most of the time reindex-from-remote isn't clearing the
scrolls that it uses. This isn't the end of the world because we
flush old scroll contexts after a while but this isn't great.

Noticed while experimenting with #22514.

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

@javanna, can you have a look at this one? It is another thing with reindex-from-remote and being super careful with the client.

search:
index: new_twitter
type: tweet
- match: { hits.total: 1 }

This comment has been minimized.

Copy link
@javanna

javanna Jan 10, 2017

Member

what happened to all of these tests? They are already in the docs so we can remove them from here?

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Author Contributor

Yeah. Sorry, I meant to leave that comment. I made those before we ran the docs snippets. I should have removed them in another PR but I figured I could leave a comment and defend it. I promptly forgot.

This comment has been minimized.

Copy link
@javanna

javanna Jan 10, 2017

Member

ok I assume that we got test coverage besides docs tests and those ones were copied from the docs just to make sure that the docs were correct, which is not needed anymore?

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Author Contributor

Right!

@javanna
Copy link
Member

left a comment

LGTM

Fix reindex from remote clearing scroll
Reindex-from-remote had a race when it tried to clear the scroll. It
first starts the request to clear the scroll and then submits a task
to the generic threadpool to shutdown the client. These two things
race and, in my experience, closing the scroll generally loses. That
means that most of the time reindex-from-remote isn't clearing the
scrolls that it uses. This isn't the end of the world because we
flush old scroll contexts after a while but this isn't great.

Noticed while experimenting with #22514.

@nik9000 nik9000 force-pushed the nik9000:reindex_from_remote_close_carefully branch to b75edad Jan 10, 2017

@nik9000 nik9000 merged commit 78bb566 into elastic:master Jan 10, 2017

1 check was pending

elasticsearch-ci Build triggered. sha1 is merged.
Details

nik9000 added a commit that referenced this pull request Jan 10, 2017

Fix reindex from remote clearing scroll (#22525)
Reindex-from-remote had a race when it tried to clear the scroll. It
first starts the request to clear the scroll and then submits a task
to the generic threadpool to shutdown the client. These two things
race and, in my experience, closing the scroll generally loses. That
means that most of the time reindex-from-remote isn't clearing the
scrolls that it uses. This isn't the end of the world because we
flush old scroll contexts after a while but this isn't great.

Noticed while experimenting with #22514.

nik9000 added a commit that referenced this pull request Jan 10, 2017

Fix reindex from remote clearing scroll (#22525)
Reindex-from-remote had a race when it tried to clear the scroll. It
first starts the request to clear the scroll and then submits a task
to the generic threadpool to shutdown the client. These two things
race and, in my experience, closing the scroll generally loses. That
means that most of the time reindex-from-remote isn't clearing the
scrolls that it uses. This isn't the end of the world because we
flush old scroll contexts after a while but this isn't great.

Noticed while experimenting with #22514.
@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

Thanks again for reviewing @javanna!

master: 78bb566
5.x: 5fb4b56
5.2: 6fe6204

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.