potiuk commented on PR #64110: URL: https://github.com/apache/airflow/pull/64110#issuecomment-4195401375
@qianchongyang This PR has a few issues that need to be addressed before it can be reviewed — please see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). **Issues found:** - :x: **Description doesn't match code**: The PR description claims tasks remain in 'reserved' state with 'acknowledged: False', 'worker_pid: None', and 'time_start: None' — a task execution/dispatch problem. However, the only code change is in the duplicate-worker detection guard inside `worker()` in `celery_command.py` (lines ~225–233). This guard raises `SystemExit` when another worker with the same hostname is already running. It has no effect on how tasks are dispatched to or executed by a running worker. The stated fix cannot resolve the described problem. - :x: **Missing regression test**: No test is added for this bug fix. There should be a regression test that fails without the fix and passes with it, covering the custom-hostname worker scenario. The existing celery CLI tests should have a case exercising this duplicate-hostname check with a hostname containing an `@` sign. - :warning: **Logically redundant / incorrect fix**: The new condition is `f'@{args.celery_hostname}' in name or name.endswith(f'@{args.celery_hostname}')`. Because `endswith(x)` implies `x in name`, the `or name.endswith(...)` branch is dead code. The effective change is simply switching from `endswith` to `in`, but this makes the duplicate-worker check broader in a way that could produce false positives (e.g., a hostname 'host' matching a worker named 'celery@otherhost' if '@host' happened to appear as a substring). More importantly, the original `endswith` already correctly matched Celery's standard `workername@hostname` format, so the premise that the original logic was broken for custom hostnames is not demonstrated. - :warning: **No Gen-AI disclosure**: The PR description does not disclose whether generative AI tooling was used to co-author it. Given the mismatch between the described problem (task execution stuck in 'reserved' state) and the actual fix (modifying a duplicate-worker startup guard), this is a strong signal of unreviewed AI-generated code. Per Apache Airflow contributing guidelines, Gen-AI usage must be explicitly disclosed. **What to do next:** - The comment informs you what you need to do. - Fix each issue, then mark the PR as "Ready for review" in the GitHub UI - but only after making sure that all the issues are fixed. - There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. - Maintainers will then proceed with a normal review. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack). -- 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]
