Adding a retry logic for Hubspot associations loading#318
Conversation
| if request.status_code >= 200 and request.status_code < 300: | ||
| break | ||
| self.log.info( | ||
| "Status {0} - Waiting {2}s and trying again. Response:\n\n{1}".format( |
There was a problem hiding this comment.
[Style] The log message says "trying again" even on the final attempt, which is misleading. Consider checking try_number < retries to vary the message, or simply omit "trying again" from the final-attempt log.
PR #318 Review — HubSpot association retry logicOverall: The retry mechanism is a sensible addition, but has one blocking bug and one style issue. Blocking:
Style:
No issues found with:
|
|
Claude review cost: $0.2353 · 10 turns · 137s · |
Co-authored-by: gemma-claude-assistant[bot] <262789552+gemma-claude-assistant[bot]@users.noreply.github.com>
| data=payload, | ||
| ) | ||
| # Bad requests are retried in the while loop | ||
| if request.status_code >= 200 and request.status_code < 300: |
There was a problem hiding this comment.
[Suggestion] The retry fires on all non-2xx responses, including permanent 4xx client errors (400 Bad Request, 401 Unauthorized, 403 Forbidden). Retrying those wastes ~90 seconds per failed batch with no chance of success. Consider limiting retries to retriable codes only:
| if request.status_code >= 200 and request.status_code < 300: | |
| if request.status_code >= 200 and request.status_code < 300: | |
| break | |
| retriable = request.status_code == 429 or request.status_code >= 500 | |
| if not retriable: | |
| break |
Then the assert below will still surface the error correctly.
| # Connection errors are retried in the while loop | ||
| except requests.exceptions.ConnectionError as e: | ||
| self.log.info( | ||
| "Connection error on attempt {0}/{1}: {2} - Waiting {3}s and trying again.".format( |
There was a problem hiding this comment.
[Style] The log message always says "Waiting {3}s and trying again" but on the final attempt (try_number >= retries) the exception is re-raised immediately — no sleep, no retry. Same misleading wording applies to the HTTP-error log at the line above (self.log.info("Status {0} - Waiting {2}s and trying again...")) on the last iteration. Both should conditionally omit "trying again" on the last attempt to avoid confusing operators reading logs.
PR 318 Review — HubSpot association retry logicOverall: The approach is sound — wrapping the association batch request in a retry loop with exponential-ish back-off addresses real transient failures. Two issues worth addressing:
Everything else looks correct: |
|
Claude review cost: $0.2284 · 8 turns · 149s · |
At one of our customers the loading pipeline receives a lot of connection errors for one of their endpoints. After some research I found that this is likely because of the high amount of data loaded by this endpoint in combination with associations that again have an high amount in rows. This leads to many API calls, increasing the chance of hitting a connection error.
With this PR I introduce a retry logic for the associations loading similar to what is already implemented for the property loading but with adding a catch for connection errors.