gemini-code-assist[bot] commented on code in PR #39196:
URL: https://github.com/apache/beam/pull/39196#discussion_r3509598790
##########
infra/enforcement/sending.py:
##########
@@ -59,25 +59,71 @@ def __init__(self, logger: logging.Logger, github_token:
str, github_repo: str,
self.logger = logger
self.github_api_url = "https://api.github.com";
- def _make_github_request(self, method: str, endpoint: str, json:
Optional[dict] = None) -> requests.Response:
+ def _make_github_request(self, method: str, endpoint: str, json:
Optional[dict] = None, params: Optional[dict] = None) -> requests.Response:
"""
- Makes a request to the GitHub API.
+ Makes a request to the GitHub API with retry logic for transient
errors and rate limiting.
Args:
method (str): The HTTP method to use (e.g., "GET", "POST",
"PATCH").
endpoint (str): The API endpoint to call.
json (Optional[dict]): The JSON payload to send with the request.
+ params (Optional[dict]): The URL parameters to send with the
request.
Returns:
requests.Response: The response from the API.
"""
+ import time
url = f"{self.github_api_url}/{endpoint}"
- response = requests.request(method, url, headers=self.headers,
json=json)
+ max_retries = 5
+ backoff = 2
- if not response.ok:
- self.logger.error(f"Failed GitHub API request to {endpoint}:
{response.status_code} - {response.text}")
- response.raise_for_status()
-
+ for attempt in range(max_retries):
+ try:
+ response = requests.request(method, url, headers=self.headers,
json=json, params=params)
+
+ # Check for rate limiting / secondary rate limit (403, 429)
+ if response.status_code in [403, 429]:
+ retry_after = response.headers.get("Retry-After")
+ if retry_after:
+ sleep_seconds = int(retry_after)
+ else:
+ sleep_seconds = backoff
+
+ self.logger.warning(
+ f"GitHub API rate limit hit ({response.status_code})
on {endpoint}. "
+ f"Retrying in {sleep_seconds} seconds... (Attempt
{attempt + 1}/{max_retries})"
+ )
+ time.sleep(sleep_seconds)
+ backoff *= 2
+ continue
+
+ # Check for transient server errors (500, 502, 503, 504)
+ if response.status_code in [500, 502, 503, 504]:
+ self.logger.warning(
+ f"GitHub API server error ({response.status_code}) on
{endpoint}. "
+ f"Retrying in {backoff} seconds... (Attempt {attempt +
1}/{max_retries})"
+ )
+ time.sleep(backoff)
+ backoff *= 2
+ continue
+
+ if not response.ok:
+ self.logger.error(f"Failed GitHub API request to
{endpoint}: {response.status_code} - {response.text}")
+ response.raise_for_status()
+
+ return response
+ except requests.exceptions.RequestException as e:
+ if attempt == max_retries - 1:
+ raise
+ self.logger.warning(
+ f"GitHub API request exception on {endpoint}: {e}. "
+ f"Retrying in {backoff} seconds... (Attempt {attempt +
1}/{max_retries})"
+ )
+ time.sleep(backoff)
+ backoff *= 2
Review Comment:
Currently, any non-transient client error (such as `400 Bad Request`, `401
Unauthorized`, or `404 Not Found`) will raise an `HTTPError` via
`response.raise_for_status()`, which is then caught by the generic `except
requests.exceptions.RequestException` block and retried up to 5 times.
Retrying non-transient client errors is inefficient, wastes API quota, and
unnecessarily delays workflow execution since these errors will not resolve on
subsequent retries. We should immediately raise the exception for non-transient
client errors (4xx status codes, excluding rate limits/timeouts like 403, 408,
and 429).
```suggestion
except requests.exceptions.RequestException as e:
if isinstance(e, requests.exceptions.HTTPError) and
e.response is not None:
if e.response.status_code not in [403, 408, 429] and
e.response.status_code < 500:
raise
if attempt == max_retries - 1:
raise
self.logger.warning(
f"GitHub API request exception on {endpoint}: {e}. "
f"Retrying in {backoff} seconds... (Attempt {attempt +
1}/{max_retries})"
)
time.sleep(backoff)
backoff *= 2
```
##########
infra/enforcement/sending.py:
##########
@@ -59,25 +59,71 @@ def __init__(self, logger: logging.Logger, github_token:
str, github_repo: str,
self.logger = logger
self.github_api_url = "https://api.github.com";
- def _make_github_request(self, method: str, endpoint: str, json:
Optional[dict] = None) -> requests.Response:
+ def _make_github_request(self, method: str, endpoint: str, json:
Optional[dict] = None, params: Optional[dict] = None) -> requests.Response:
"""
- Makes a request to the GitHub API.
+ Makes a request to the GitHub API with retry logic for transient
errors and rate limiting.
Args:
method (str): The HTTP method to use (e.g., "GET", "POST",
"PATCH").
endpoint (str): The API endpoint to call.
json (Optional[dict]): The JSON payload to send with the request.
+ params (Optional[dict]): The URL parameters to send with the
request.
Returns:
requests.Response: The response from the API.
"""
+ import time
url = f"{self.github_api_url}/{endpoint}"
- response = requests.request(method, url, headers=self.headers,
json=json)
+ max_retries = 5
+ backoff = 2
- if not response.ok:
- self.logger.error(f"Failed GitHub API request to {endpoint}:
{response.status_code} - {response.text}")
- response.raise_for_status()
-
+ for attempt in range(max_retries):
+ try:
+ response = requests.request(method, url, headers=self.headers,
json=json, params=params)
+
+ # Check for rate limiting / secondary rate limit (403, 429)
+ if response.status_code in [403, 429]:
+ retry_after = response.headers.get("Retry-After")
+ if retry_after:
+ sleep_seconds = int(retry_after)
+ else:
+ sleep_seconds = backoff
Review Comment:
The `Retry-After` header returned by APIs can occasionally be malformed,
empty, or formatted as an HTTP date string instead of an integer. Attempting to
directly cast it using `int(retry_after)` without handling potential exceptions
can lead to an unhandled `ValueError` that crashes the workflow.
It is safer to wrap the conversion in a `try-except` block and fall back to
the default `backoff` value if parsing fails.
```suggestion
retry_after = response.headers.get("Retry-After")
try:
sleep_seconds = int(retry_after) if retry_after else
backoff
except ValueError:
sleep_seconds = backoff
```
##########
infra/enforcement/sending.py:
##########
@@ -97,13 +143,13 @@ def _send_email(self, title: str, body: str, recipient:
str) -> None:
def _get_open_issues(self, title: str) -> List[GitHubIssue]:
"""
- Retrieves the number of open GitHub issues with a given title.
+ Retrieves the open GitHub issues with a given title.
Args:
title (str): The title of the GitHub issue.
"""
- endpoint =
f"search/issues?q=is:issue+repo:{self.github_repo}+in:title+{title}+is:open"
- response = self._make_github_request("GET", endpoint)
+ q = f'is:issue repo:{self.github_repo} in:title "{title}" is:open'
Review Comment:
If the `title` parameter contains double quotes (e.g., `[SECURITY] Action
Required: "Unmanaged" Keys`), constructing the query string directly will
result in a malformed GitHub search query. This can cause the search to fail or
return incorrect results.
To prevent this, we should escape any double quotes in the `title` before
embedding it in the query string.
```suggestion
escaped_title = title.replace('"', '\\"')
q = f'is:issue repo:{self.github_repo} in:title "{escaped_title}"
is:open'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
