bugraoz93 commented on PR #44332: URL: https://github.com/apache/airflow/pull/44332#issuecomment-2545723758
>* Can you check that limit is actually being respected? In my manual testing, I was trying to only fetch 14 runs but I always got the default of 100. @pierrejeambrun would you have any ideas here? >* Can we add a test case for a triply nested dag group? Because it looks like we're only bubbling a failed state up twice. Thanks for the comment! I will include test cases for more nested groups and test the limit accordingly. >A few things to adjust before we can merge. Also some part of the logic are re-implementation (state propagation bug mentioned by Brent), I would suggest to try to keep things similar to the legacy implementation that has proven to be correct for many edge cases. Unless some stuff need improving of course, but the legacy code seems cleaner to me at this point. Maybe just because i'm more used to it. Thanks for the detailed review! I found the legacy implementation more complex to understand, to be honest :sweat_smile: For example, I couldn't easily grasp these edge cases (mostly for `Mapped` objects) which didn't included in couple of iterations. We have covered most edge cases here too. Sorry if methods without comments made any confusion and took your time more while reviewing! I am not against converting the new logic to legacy implementation. Which area would you like to see the legacy implementation? I copied most of the logic from the legacy code to here other than building up the `task map` and decoupling the logic into multiple methods which was mostly populated under `dag_to_grid`. I will check these comments in detail today and start making the changes. -- 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]
