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

TDL-26714: Fix ratelimit error and add retry for 5xx errors #155

Merged
merged 11 commits into from
Nov 27, 2024

Conversation

prijendev
Copy link
Contributor

@prijendev prijendev commented Nov 20, 2024

Description of change

  • Add backoff for 5xx errors.
  • Reduce concurrent request limit to 20.
  • Make a maximum of 450 ticket_audit(an endpoint specific rate limit) API calls per minute. If it hits 450 call within a minute, then wait for the remaining seconds.

Manual QA steps

  • Run the sync mode and verify that the tap hits a max of 20 concurrent requests at a time.
  • Run the sync mode and verify that tap retries on any 5xx errors.

Risks

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

try:
response_json = await response.json()
except (ContentTypeError, ValueError) as e:
LOGGER.warning("Error decoding response from API. Exception: %s", e, exc_info=True)
Copy link
Contributor Author

@prijendev prijendev Nov 20, 2024

Choose a reason for hiding this comment

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

For every non-200 error, API does not return a valid JSON response. Showing, logger with stack trace make it very ugly.

@prijendev prijendev changed the title TDL-26714: Add retry for 5xx errors TDL-26714: Fix ratelimit error and add retry for 5xx errors Nov 23, 2024
@@ -327,6 +331,15 @@ def emit_sub_stream_metrics(sub_stream):
ticket_ids = []
# Write state after processing the batch.
singer.write_state(state)
counter+=CONCURRENCY_LIMIT
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
counter+=CONCURRENCY_LIMIT
counter += CONCURRENCY_LIMIT

Comment on lines 339 to 340
# Add 2 seconds of buffer time
time.sleep(max(0, 60 - (time.time() - start_time)+2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Though original code logic is correct, suggested code may offer better readability, clarity and code maintenance.

Suggested change
# Add 2 seconds of buffer time
time.sleep(max(0, 60 - (time.time() - start_time)+2))
# Calculate elapsed time
elapsed_time = time.time() - start_time
# Calculate remaining time until the next minute, plus buffer
remaining_time = max(0, 60 - elapsed_time + 2)
# Sleep for the calculated time
time.sleep(remaining_time)

Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Left some suggestiosn.

@prijendev
Copy link
Contributor Author

Left some suggestiosn.

Addressed all your comments

@prijendev prijendev merged commit a40fd4c into master Nov 27, 2024
5 checks passed
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.

3 participants