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.

Sign up for GitHub

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

Jump to bottom

[Feature] The code should retry fetching the data if the connection fails #13

Open
DragonWarrior15 opened this issue May 8, 2021 · 6 comments · May be fixed by #16
Open

[Feature] The code should retry fetching the data if the connection fails #13

DragonWarrior15 opened this issue May 8, 2021 · 6 comments · May be fixed by #16

Comments

@DragonWarrior15
Copy link
Contributor

Currently the code does not support any kind of retry in order to call the api again to fetch the results. The following piece of code might work for this (this will be wrapped around the base api call so that modifications are needed at only one place)

success = False
for i in range(Constants.max_retries):
    if(success):
        break
    try:
        some_result = api_call()
        success = True
    except Exception as e:
        # wait few sec and continue
        time.sleep(Constants.sleep_time)
        continue

We could keep Constants.max_retries as 3 and Constants.sleep_time as 2.
I am not sure if this is the best way to do this, but if this is something useful, I can add it to the code with a PR. This is useful in cases where the API may not be reachable, or when any part of the package, or an external user is running a loop which calls the api multiple times.
@DragonWarrior15 DragonWarrior15 changed the title [Bug] The could should retry fetching the data if the connection fails [Bug] The code should retry fetching the data if the connection fails May 8, 2021
@backtrackbaba backtrackbaba changed the title [Bug] The code should retry fetching the data if the connection fails [Feature] The code should retry fetching the data if the connection fails May 8, 2021
@backtrackbaba
Copy link
Owner

There are actually two lines of thought on this feature: retry, rate limit, and other such things should be a part of the library or should be taken care of by the consumer.

Instead of using a for loop, an alternate approach could be by using some internal configurations in the comment above.

We are using requests library for our API calls and it internally uses urllib3.

In urllib3/requests, you can configure the number of retries you want, the response codes on which you want to retry, an exponential backoff factor, and so on. You can read more about the same here

In this case, I feel handling the retry could be a part of the library itself. Let me know you views

@DragonWarrior15
Copy link
Contributor Author

This will take a while for me to get through. We also have to consider the changes to the api now, mentioned in the news here. Per IP there is a limit of 100 request per 5 minutes.

@DragonWarrior15
Copy link
Contributor Author

DragonWarrior15 commented May 8, 2021 •
edited
Loading

All right, we should at least implement the retry based on the web post you have linked above. The default settings seem to make sense for our purpose.

Another thing linked with all of this is the following: the code in the base api can return an error as well. How should I handle it in the code here, essentially, what would be the right syntax to check that a dict (even empty) has been returned ? Will an isinstance check suffice ?

@backtrackbaba
Copy link
Owner

There are two changes linked to this feature: 1) Rate Limiting & 2) Raising Custom Exception

Rate Limiting is something that can be implemented right away as it's not a breaking change. On the other hand, Raising Custom Exceptions is something that would be a breaking change as the consumers of the current implementation would have to do some additional changes to take care of it.

Regarding changes to the code to check the right values would be a part of the changes involving custom exceptions. Over there, you could have some simple checks like a) Applying filters only if the list of centers is more than 1, b) Raising a custom exception wherever needed, and so on.

You can go ahead with the implementation for retry logic and we can push it to Pypi after the dependant changes are complete.

@DragonWarrior15 DragonWarrior15 linked a pull request May 8, 2021 that will close this issue
Open
@DragonWarrior15
Copy link
Contributor Author

Should 403 also be a part of the list where retry is called ? I guess a lot of people are facing 403 due to so many requests to the API.

@DragonWarrior15
Copy link
Contributor Author

Bad idea to do this, ignore based on real world experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature retry DragonWarrior15/cowin
2 participants
@backtrackbaba @DragonWarrior15