-
Notifications
You must be signed in to change notification settings - Fork 45
fix(cdk): improve rate limiting error messages to suggest reducing num_workers #949
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
13c6f47
cb4514e
d12c6ee
7301186
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,7 +222,8 @@ def get_error_handler(self) -> Optional[ErrorHandler]: | |
| send_mock = mocker.patch.object(requests.Session, "send", return_value=req) | ||
|
|
||
| with pytest.raises( | ||
| AirbyteTracedException, match="Exception: HTTP Status Code: 429. Error: Too many requests." | ||
| AirbyteTracedException, | ||
| match="Rate limit exceeded \\(HTTP status code 429\\). Try decreasing the number of workers to stay within API rate limits.", | ||
| ): | ||
| list(stream.read_records(SyncMode.full_refresh)) | ||
| if retries <= 0: | ||
|
|
@@ -316,7 +317,7 @@ def test_raise_on_http_errors_off_429(mocker): | |
| mocker.patch.object(requests.Session, "send", return_value=req) | ||
| with pytest.raises( | ||
| AirbyteTracedException, | ||
| match="Exhausted available request attempts. Please see logs for more details. Exception: HTTP Status Code: 429. Error: Too many requests.", | ||
| match="Rate limit exceeded \\(HTTP status code 429\\). Try decreasing the number of workers to stay within API rate limits.", | ||
| ): | ||
| stream.exit_on_rate_limit = True | ||
| list(stream.read_records(SyncMode.full_refresh)) | ||
|
Comment on lines
318
to
323
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new retry log message no longer includes the underlying exception text (previously
str(exc)), which can remove useful context like the request URL / error details when diagnosing repeated 429s. Consider appendingstr(exc)(or key fields like request URL) to this log line while keeping the actionable guidance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — restored
str(exc)asLast error: {str(exc)}in the log line so debug context is preserved alongside the new guidance. Fixed in 7301186.Devin session