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

Increase pelias timeouts during import step #199

Closed
wants to merge 3 commits into from
Closed

Increase pelias timeouts during import step #199

wants to merge 3 commits into from

Conversation

wcedmisten
Copy link
Contributor

No description provided.

@michaelkirk
Copy link
Member

Thanks for sharing this! I know this is intended as just a demonstration to get people past the build process, but if you're interested in helping further...

The pelias.json config is used in two places:

  1. The import process
  2. The runtime containers for the geocoding service

In my experience the increase in timeout is necessary for the import process, but not the runtime containers. I'd prefer to have the "default" timeout for runtime and only the mega-long one on the import process.

My plan is to eventually implement something like that, but if you are interested in contributing, I'd be happy to merge it.

@wcedmisten
Copy link
Contributor Author

What are your thoughts on reverting the pelias.json change for now and only merging the wait.sh change? My understanding is wait.sh is only used during the import, so it wouldn't affect the runtime timeout.

This should still resolve the issue I had with the import timing out.

@michaelkirk
Copy link
Member

I'm open to it, but 600 is a 20x jump. Does it need to be so large? How did you arrive at that number?

@wcedmisten
Copy link
Contributor Author

I just used the same 10 minute timeout as mentioned here pelias/docker#217 (comment), although in a different context. It was fairly arbitrary. I wasn't too concerned with losing a few extra minutes compared to the many hours of total import time for the dataset I was using.

@wcedmisten wcedmisten marked this pull request as ready for review December 10, 2022 20:45
@michaelkirk
Copy link
Member

I just used the same 10 minute timeout as mentioned here pelias/docker#217 (comment),

That number was something I made up as evidence that a timeout was occurring at all, and should definitely not be considered a bar for mergeable code.

How about adding some debug output to print how long it actually takes to succeed?

It's nice to make the code more robust to failures, but increasing timeouts arbitrarily kind of defeats the purpose of timeouts.

@michaelkirk
Copy link
Member

I started occasionally seeing a similar timeout in CI during peak hours.

I changed the retry logic to be based on time rather than on the number of attempts (though they are approximately the same thing in this case).

I also started logging how long it takes when successful. The slowest runtime I measured was in the mid 20's, but presumably sometimes it's more than that, or else I wouldn't have been hitting the timeout, so I bumped it up to 60 seconds for now.

015497d

Let me know if you continue to hit that timeout.

@wcedmisten
Copy link
Contributor Author

Thanks for your work on this! I'll let you know if I run the import again!

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

Successfully merging this pull request may close these issues.

2 participants