Copilot commented on code in PR #64110:
URL: https://github.com/apache/airflow/pull/64110#discussion_r3025333722
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -225,8 +225,13 @@ def worker(args):
active_workers = inspect.active_queues()
if active_workers:
active_worker_names = list(active_workers.keys())
- # Check if any worker ends with @hostname
- if any(name.endswith(f"@{args.celery_hostname}") for name in
active_worker_names):
+ # Check if any worker matches the specified hostname
+ # Celery worker names are in format: {worker_name}@{hostname}
+ # The hostname portion should contain our specified hostname
+ if any(
+ f"@{args.celery_hostname}" in name or
name.endswith(f"@{args.celery_hostname}")
+ for name in active_worker_names
+ ):
Review Comment:
This change only adjusts the duplicate-worker hostname detection in the
`airflow celery worker` CLI path. It doesn’t touch CeleryExecutor task
dispatch/worker association logic, so it’s unclear how it resolves the reported
behavior of tasks staying reserved and never executing with custom
`--celery-hostname`. Either update the PR description to match the actual fix,
or add the missing executor-side changes that address the reserved-task issue
(with a regression test).
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -225,8 +225,13 @@ def worker(args):
active_workers = inspect.active_queues()
if active_workers:
active_worker_names = list(active_workers.keys())
- # Check if any worker ends with @hostname
- if any(name.endswith(f"@{args.celery_hostname}") for name in
active_worker_names):
+ # Check if any worker matches the specified hostname
+ # Celery worker names are in format: {worker_name}@{hostname}
+ # The hostname portion should contain our specified hostname
+ if any(
+ f"@{args.celery_hostname}" in name or
name.endswith(f"@{args.celery_hostname}")
+ for name in active_worker_names
+ ):
Review Comment:
The updated hostname-matching behavior is not covered by existing unit tests
(current tests for this module assert the worker_main options but don’t
exercise the duplicate-worker detection against `inspect.active_queues()`
results, including cases like `--celery-hostname host` vs existing
`celery@host2`, and `--celery-hostname myworker@%h` templates). Please add a
regression test that mocks `inspect.active_queues()` and verifies the CLI
raises (or does not raise) `SystemExit` for these scenarios.
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -225,8 +225,13 @@ def worker(args):
active_workers = inspect.active_queues()
if active_workers:
active_worker_names = list(active_workers.keys())
- # Check if any worker ends with @hostname
- if any(name.endswith(f"@{args.celery_hostname}") for name in
active_worker_names):
+ # Check if any worker matches the specified hostname
+ # Celery worker names are in format: {worker_name}@{hostname}
+ # The hostname portion should contain our specified hostname
+ if any(
+ f"@{args.celery_hostname}" in name or
name.endswith(f"@{args.celery_hostname}")
+ for name in active_worker_names
+ ):
Review Comment:
The new worker-existence check uses substring matching
(`f"@{args.celery_hostname}" in name`), which can report false positives (e.g.,
`args.celery_hostname="host"` will match an existing worker `celery@host2`) and
still fails to match common valid inputs where `--celery-hostname` is a full
nodename/template (e.g., `myworker@%h` won’t match `myworker@actual-host`).
Consider normalizing to a concrete expected nodename first (expand `%h`/`%n` if
supported), then compare for exact equality; alternatively, if you intend
`--celery-hostname` to be host-only, compare against the host segment of
`name.split("@", 1)[1]` for exact match rather than substring checks. Also, `or
name.endswith(...)` is redundant once `in` is used.
```suggestion
cli_hostname = args.celery_hostname
# Celery worker names are in format: {worker_name}@{hostname}
# If the CLI argument includes "@", treat it as a full worker
name; otherwise match on the host segment.
if "@" in cli_hostname:
conflict = any(name == cli_hostname for name in
active_worker_names)
else:
conflict = any(
"@" in name and name.split("@", 1)[1] == cli_hostname
for name in active_worker_names
)
if conflict:
```
--
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]
