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

Remove buggy indices.exists() calls from code #1580

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

DJRickyB
Copy link
Contributor

@DJRickyB DJRickyB commented Sep 13, 2022

Recently we've had a few errors surface in Rally executions when indices/data-streams are deleted. The errors would be reported as connection timed out for the DELETE /<index> call, but when inspected (via a packet capture) the server was shown to have responded correctly and in a timely fashion to Rally.

It turns out that in our version of elasticsearch-py, indices.exists() calls were handled via GET rather than HEAD. It's unclear to me why this causes an issue, but any of the following fixes the issue when targeting very large indices with the delete-index operation:

  • Reduce the body size of the index (e.g. reduce the number of mappings)
  • Set only-if-exists to false in the operation (to avoid the exists() call)
  • Change the exists() call to a get() call.

I'm led to believe the bytes from the body are left in the event loop and not dequeued properly by the client, hence blocking somehow the response from the very next transaction.

This PR changes from using the exists() method to using the get() method to determine an index's existence.

A test is updated to show the difference in behavior under-the-hood.

@DJRickyB DJRickyB self-assigned this Sep 13, 2022
@DJRickyB DJRickyB added the bug Something's wrong label Sep 13, 2022
@DJRickyB DJRickyB added this to the 2.6.1 milestone Sep 13, 2022
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

It's ok by me! What a wild ride. Probably worth a comment about why using get instead of exists and maybe even an assertion that you are on the un-fixed version of the client. Once you upgrade I think you want to revert this commit, right?

@DJRickyB
Copy link
Contributor Author

@elasticmachine run rally/rally-tracks-compat

Copy link
Contributor

@michaelbaamonde michaelbaamonde left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks so much for chasing this down.

Another point in favor of prioritizing the client upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's wrong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants