Copilot commented on code in PR #53368:
URL: https://github.com/apache/airflow/pull/53368#discussion_r2644067916
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py:
##########
@@ -115,7 +115,7 @@ def __init__(
completion_mode: str | None = None,
completions: int | None = None,
manual_selector: bool | None = None,
- parallelism: int | None = None,
+ parallelism: int = 1,
Review Comment:
Changing the default value of `parallelism` from `None` to `1` is a breaking
change that could affect existing users. Users who previously relied on
`parallelism=None` to have different behavior may experience unexpected
changes. This should be documented in a newsfragment file as mentioned in the
PR guidelines, particularly in a `.significant.rst` file to notify users of
backwards incompatible changes.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py:
##########
@@ -461,7 +452,9 @@ def get_pods(
pod_list: Sequence[k8s.V1Pod] = []
retry_number: int = 0
- while len(pod_list) != self.parallelism or retry_number <=
self.discover_pods_retry_number:
+ while retry_number <= self.discover_pods_retry_number:
+ if len(pod_list) == self.parallelism:
+ break
pod_list = self.client.list_namespaced_pod(
namespace=pod_request_obj.metadata.namespace,
label_selector=label_selector,
Review Comment:
The loop logic has a potential issue where `pod_list` is assigned inside the
loop after checking its length. This means that if the initial `pod_list` is
empty (as initialized on line 452), the first iteration will always execute the
list operation, which is correct. However, if fewer pods than expected are
found, the loop continues retrying without any delay, which could result in
rapid API calls. Consider adding a sleep between iterations to avoid
overwhelming the Kubernetes API server.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py:
##########
@@ -461,7 +452,9 @@ def get_pods(
pod_list: Sequence[k8s.V1Pod] = []
retry_number: int = 0
- while len(pod_list) != self.parallelism or retry_number <=
self.discover_pods_retry_number:
+ while retry_number <= self.discover_pods_retry_number:
+ if len(pod_list) == self.parallelism:
+ break
pod_list = self.client.list_namespaced_pod(
namespace=pod_request_obj.metadata.namespace,
label_selector=label_selector,
Review Comment:
The error message at line 465 states "No pods running with labels" but this
might not be accurate. The pods could exist but not be in a running state yet
(e.g., pending, initializing). Consider updating the error message to be more
accurate, such as "Failed to find expected number of pods with labels" or "No
pods found with labels" to avoid confusion.
--
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]
