Add retry logic to BookStackApiClient with backoff. #4622
Conversation
|
@tasosger is attempting to deploy a commit to the Danswer Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds retry logic with exponential backoff to the BookStackApiClient to handle temporary service disruptions (HTTP 502, 503, 504 errors), addressing issue #4381 where Bookstack indexing would fail during temporary outages.
- Added
max_retries(default: 5) andbackoff_factor(default: 2.0) parameters toBookStackApiClientinitialization - Implemented retry loop in
getmethod with exponential backoff for retryable status codes - Found potential infinite loop bug due to incorrect comparison operator (
<=instead of<in retry loop) - Identified debug print statement that needs to be removed
- Detected duplicate backoff calculation that should be consolidated
The changes effectively solve the core issue while maintaining code quality standards, though the identified issues should be addressed before merging.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
1 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| def get(self, endpoint: str, params: dict[str, str]) -> dict[str, Any]: | ||
| url: str = self._build_url(endpoint) | ||
| headers = self._build_headers() | ||
| response = requests.get(url, headers=headers, params=params) | ||
| retries = 0 | ||
| while retries <= self.max_retries: |
There was a problem hiding this comment.
logic: Condition retries <= self.max_retries will cause one extra retry attempt beyond max_retries. Should be < instead of <=
| while retries <= self.max_retries: | |
| while retries < self.max_retries: |
| time.sleep(sleep_time) | ||
| except requests.RequestException as e: | ||
| retries += 1 | ||
| print(retries) |
There was a problem hiding this comment.
style: Remove debug print statement
| print(retries) |
| sleep_time = self.backoff_factor * (2 ** (retries - 1)) + random.uniform(0, 1) | ||
| time.sleep(sleep_time) | ||
| except requests.RequestException as e: | ||
| retries += 1 | ||
| print(retries) | ||
| if retries > self.max_retries: | ||
| raise BookStackClientRequestFailedError(503, str(e)) | ||
| sleep_time = self.backoff_factor * (2 ** (retries - 1)) + random.uniform(0, 1) | ||
| time.sleep(sleep_time) |
There was a problem hiding this comment.
style: Duplicate backoff calculation. Consider extracting to a helper method to avoid repetition
|
|
||
| try: | ||
| json = response.json() | ||
| except Exception: |
There was a problem hiding this comment.
style: Catching bare Exception is too broad. Consider catching json.JSONDecodeError instead
Description
Basic code to handle retries for certain response status codes (502,503,504), in the Bookstack client.py, in response to the #4381 issue.
How Has This Been Tested?
This functionality has been tested using unit tests with mocked HTTP responses to simulate various scenarios, including successful responses, client errors (e.g., 401), server errors (e.g., 500), and retryable errors (502 ,503, 504). The requests.get method was patched using unittest.mock.patch to control the responses and validate that the BookStackApiClient correctly handles retries, raises appropriate errors, and returns the expected results. Each test case checks specific behaviors, such as retry logic, error handling, and success recovery.
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.