kaxil commented on PR #69864: URL: https://github.com/apache/airflow/pull/69864#issuecomment-5706905209
Everything from my last round is addressed. I re-checked the three open comments at `733cfaa8` myself, so here is the whole set in one place with what it would take to close it out. **One thing holds my approval: the counter at line 500.** I applied the one-line change (move `self.count += 1` below the repeat early-return) and measured both sides in breeze at this HEAD. Redacted mode, a Dag where every task changes `retries` plus one `/dag/timezone` change: | case | today | with the counter moved | |---|---|---| | 600 tasks, `max_changes=500` (the default) | `truncated: true`, 1 record, timezone absent | `truncated: false`, both paths, exact counts | | 5000 tasks, `max_changes=5000` (`MAX_ALLOWED_CHANGES`) | `truncated: true`, 1 record, timezone absent | `truncated: false`, both paths, exact counts | At the default bound a 600-task fan-out already hides the timezone change, and at 5000 tasks nothing a caller may pass recovers it, because 5000 is the ceiling. So for "bumped `retries` in `default_args` and changed the schedule" on a large Dag, the operator sees one record saying retries changed on `*` tasks plus `truncated: true`, and the schedule change is gone with nothing pointing at it. That is the shape the endpoint PR freezes. The change costs one test parametrization. Engine plus model suites with it applied: `1 failed, 621 passed`, and the failure is `test_build_diff_excludes_new_group_beyond_change_limit[False-2]`, which under the change returns both records and `truncated: false`. I think that test is the one to move, because the suite currently pins both answers. `test_build_diff_bound_withholds_records_not_public_paths` asserts `redacted["truncated"] is False` at `max_changes=1` for ten occurrences of a single path, with the comment "no bound can withhold a path from the caller or cost an occurrence". `test_build_diff_excludes_new_group_beyond_change_limit[False-2]` is that same situation with a second public path present, and there the bound does withhold one. Whether a later distinct path survives depends on how many occurrences the earlier paths happened to have, even though those occurrences produced no records. Keeping the current counting is a reasonable call to make instead. In that case the docstring at lines 112-124 and the name of `test_build_diff_bound_withholds_records_not_public_paths` both need to say that a bound can withhold a public path, since today they say it cannot. **The other two comments are optional and will not hold approval.** The canonicalization handler logging only `error_type` costs you the `unsupported client_defaults sections: [...]` message you added two rounds ago, and the unreachable `serialized_dag_recursion_limit_exceeded` is a wire-contract question the CLI and endpoint PRs will inherit from line 53. Take them or leave them. Nothing else from me. I checked the reason list against every `_mark_unavailable` call site and the two `get_diff` adds (complete and accurate apart from the unreachable one), `get_diff` itself (one query with `joinedload`/`selectinload`, bound validated before the lookup, no writes), and the `serialized_objects.py` refactor (`_DAG_CALLBACK_FIELDS` iteration order cannot reach `dag_hash` or the stored blob, both of which go through `json.dumps(..., sort_keys=True)`). Settle the counter either way and I approve. -- 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]
