api: Add random exponential backoff to do_api_query.

[orientor]

Set delay time on failure in do_api_query according to random exponential backoff.

Fixes #537 .

[orientor]

Made a PR for do_api_query. But base and cap need to be decided. Also if this is done, then I can change class RandomExponentialBackoff accordingly.

[timabbott]

Can you provide terminal output showing you've tested the error handling behaves correctly?

[orientor]

I have tested the condition when retry is required. But in the original code retry happens only in two cases:

1. When we receive a 50x status code.
2. If we have connected to the server before but couldn't connect now.

Hence in ALL other cases (with no retry) my code works same as the original code.(Tested it and also an error gets raised in these cases so changing loop doesn't effect them)

For the first 2 cases the output will be like that shown in the image.

[orientor]

I also checked if it works normally in normal conditions.

[orientor]

Some of the other errors:

[orientor]

I have thoroughly checked most of the cases. Code working fine in all.

[timabbott]

@orientor can you rework your commits here to follow our commit style? I think what makes sense are:

• A first commit that adjusts the behavior of RandomExponentialBackoff to add a delay_cap, with a default value of 90 (10 is too short).
• A second commit that does the delay_base refactor (nonfunctional).
• A third commit that changes the randomization approach in RandomExponentialBackoff, which we might choose to skip.
• A next commit that migrates the error_retry logic to use RandomExponentialBackoff
• Etc.

That approach will be reviewable, biseactable, and mergable.

[orientor]

@timabbott I have reworked my changes into two commits. In the first commit I am improving the Random Exponential Backoff class. In the second commit I am updating the do_api_query method to use Random Exponential Backoff.

Now going to update it to 4.

[orientor]

@timabbott removed delay_base completely as it changed only one iteration(it was set to 1/2 so it would basically reduce power of 2 by 1) and I think was not required.

Reworked changes into 3 commits.

1. Added delay_cap as class variable to the CountingBackoff class, the superclass for RandomExponentialBackoff. Changed the default delay_cap to 90.

2. Changed the algorithm for Random Exponential Backoff.

3. Changed do_api_query so as to use Random Exponential backoff.

[timabbott]

The first commit adds the delay_cap field but doesn't have it do anything. That isn't a coherent commit -- it's just confusing were to merge just that.

Zulip's development model is around every commit in every PR being mergable incrementally, which is important for bisecting as well as efficient code review.

Can you fix the first commit to fully implement delay_cap?

[orientor]

@timabbott Improved the commits. The first commit now basically adds delay_cap properly, with the Random Exponential Backoff now using delay_cap. The second commit improves Random Exponential Backoff algorithm. The third commit adds Random Exponential Backoff to do_api_query method.

[timabbott]

I merged the first commit and posted a couple comments. I'll also note that this doesn't replace the time.sleep(1) in call_on_each_event, nor does it add a change to detect "invalid authentication" failures and not retry those, which I think we should do.

[orientor]

@timabbott Changed the code to be more readable and have minimal changes. I was not aware of call_on_each_event. Will make the necessary commits soon. Also can you shed some light on invalid authentication? According to me the only way of having invalid authentication is wrong API key. Are there any other methods? I will make the changes accordingly.

[orientor]

Added exponential backoff to call_on_each_event. But the function was originally intended to keep retrying to connect the server until successful. So an infinite loop. @timabbott @showell Is that intended or should I add a max number of retries?

[zulipbot]

Heads up @orientor, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.