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

Add timeouts to sniper source config #5624

Merged
merged 3 commits into from
Sep 23, 2016
Merged

Add timeouts to sniper source config #5624

merged 3 commits into from
Sep 23, 2016

Conversation

Gobberwart
Copy link
Contributor

Short Description:

Sniper currently uses hard-coded values for timeout when trying to validate/retrieve data from sources. Some sources (eg. localhost:5000/raw_data) should have lower timeouts than others as they can be significantly slower.

This is now user-configurable per-source (default to 5 seconds) which allows users to configure higher timeouts for known slow sources, and lower timeouts for sources that should be fast.

Fixes/Resolves/Closes (please use correct syntax):

  • N/A

@mention-bot
Copy link

@Gobberwart, thanks for your PR! By analyzing the annotation information on this pull request, we identified @YvesHenri, @solderzzc and @mhdasding to be potential reviewers

@YvesHenri
Copy link

YvesHenri commented Sep 23, 2016

Hi @Gobberwart. I've explained this LOTS of times. The only source that timeouts frequently is the pokewatchers. This is one source that I found, but it doesn't mean you NEED to use it. It is ALSO the only TROUBLESOME URL among the others. It doesnt only takes a few to load, but it also provides crap data. I almost removed it from the example file. I, actually, WILL remove it because people keep asking this over and over. Sniper was meant so that you could find and use your own source.
Also, please note that just because every source has a hard-coded value of 5 seconds for their timeout, it DOES NOT mean it'll take 5 seconds each ("and lower timeouts for sources that should be fast"), am I'm sure you know that. Making it user-configurable WILL NOT help in any way, since the issue is on their service.
And, again, I do not want to put any more params in the config file, unless they're E-X-T-R-E-M-E-L-Y relevant.

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 23, 2016

Sorry, I completely disagree with you, and this is not "Invalid". I am also well aware of how timeouts work.

Setting a short timeout on a source that should be quick allows a failure to be identified quickly. And for a source that is often slow, it allows users to provide time for it to respond rather than timing out 50-90% of the time.

I cannot begin to tell you how many times I've had to modify the code of sniper.py to allow pokewatchers or pokesnipers time to respond.

As for "crap data" - that's your opinion. I'm getting good responses from pokewatchers, it just takes a while to respond sometimes. Hence why I'd like to be able to allow a longer timeout for that source. We could increase the global timeout for all sources, but then... if localhost doesn't respond within a couple of seconds, something is seriously wrong with it, so why allow it more time?

TL;DR - This is E-X-T-R-E-M-E-L-Y relevant :)

@Gobberwart
Copy link
Contributor Author

Anyone else want to weigh in on this one? Seems @YvesHenri and I don't agree, so we need an independent evaluation.

@GhosterBot
Copy link
Contributor

I also manually increase the global timeout in order for pokewatcher to respond in time, (and found a real Dragonite btw!). So finding a ways to adress this is a good idea.

Copy link
Contributor

@DBa2016 DBa2016 left a comment

Choose a reason for hiding this comment

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

I think adding a user-configurable timeout is a good idea. Some people might want to overall increase the timeout values because they sit on slow networks, others in turn want to quickly skip unresponseive sources. The config value is not mandatory anyway and the default looks sane to me.

@Gobberwart Gobberwart merged commit 9c05c96 into PokemonGoF:dev Sep 23, 2016
@Gobberwart Gobberwart deleted the gobb_snipetimeout branch November 9, 2016 22:33
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.

5 participants