SHESHU45 commented on issue #75863: URL: https://github.com/apache/airflow/issues/75863#issuecomment-6496530443
Thanks for the review @amoghrajesh **1. Migration & storage** — You're right, missing Alembic migration is a blocker. Will add one for `required_approvals` (Integer) + `responses` (JSON). On the JSON column: the row lock (`FOR UPDATE OF hitl_detail` + `populate_existing=True`) makes the append atomic within the transaction, so concurrent responses won't be lost. That said, a separate `hitl_response` table would be cleaner long-term — happy to keep JSON for v1 and file a follow-up for normalization if maintainers prefer. **2. Reject wins** — You caught a real gap. The trigger doesn't check for `Reject` before counting approvals. Will add immediate-reject logic in both the trigger (`_handle_response`) and Core API route. **3. Timeout with partial approvals** — Good catch. Currently the trigger just keeps waiting forever. I'll make timeout strict: if deadline hits with insufficient approvals, fail (or apply defaults if set) rather than waiting indefinitely. **4. Scope** — Currently `required_approvals` lives on the base model but only `ApprovalOperator` validates it. Two paths: - **A.** Keep it `ApprovalOperator`-only (matches the issue, simpler) - **B.** Make it generic on `HITLOperator` (lets `HITLBranchOperator` use quorum too) Lean toward A for v1 unless maintainers want B. Happy to move the column to operator-specific storage if that's cleaner. **5. Validation & counting** — Will add: - `required_approvals > len(assigned_users)` → `ValueError` in `__init__` - Case-insensitive "Approve" matching - `required_approvals >= 1` guard One question — any preference between A vs B? Happy to go either way. -- 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]
