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 up trappy timeouts related to ingest pipelines #112907

Conversation

DaveCTurner
Copy link
Contributor

Relates #107984

@DaveCTurner DaveCTurner added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v9.0.0 labels Sep 16, 2024
@DaveCTurner DaveCTurner requested a review from dakrone September 16, 2024 05:46
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Sep 16, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Comment on lines 76 to 79
// wait for as long as it takes to create the pipeline
MasterNodeRequest.INFINITE_MASTER_NODE_TIMEOUT,
// TODO should this be longer?
AcknowledgedRequest.DEFAULT_ACK_TIMEOUT,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only questionable bit really, should these timeouts both be infinite? Or zero for the ack timeout?

Copy link
Member

Choose a reason for hiding this comment

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

is it bad to leave it as the default of 30 seconds? Do we need to decide between 0 or infinite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it can be whatever we want, if we decide they should remain at 30s then that's fine too. IMO 30s is fairly suspicious tho, it means that if this happens to be running in a cluster that's a little slow then this will fail with a timeout, and it's not a timeout that the user can influence. But I don't have any strong opinions here.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should change it then. I suppose I'm afraid that changing it to be infinite without a way to cancel the enrichment execution would be bad, and changing it to 0 so that there's zero tolerance for a cluster to have any delay would also be bad. I don't have a strong opinion though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've set it back to 30s, introducing a new enrich-specific constant to make it clearer that this is a positive decision.

Copy link
Member

@dakrone dakrone 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 David

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 17, 2024
@elasticsearchmachine elasticsearchmachine merged commit 54435b1 into elastic:main Sep 17, 2024
15 checks passed
@DaveCTurner DaveCTurner deleted the 2024/09/16/ingest-pipeline-trappy-timeouts branch September 17, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Data Management Meta label for data/management team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants