Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AIRFLOW-6871] optimize tree view for large DAGs #7492
[AIRFLOW-6871] optimize tree view for large DAGs #7492
Changes from all commits
f57038e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much does it reduce it by? Is stripping of all ms noticable? (Could we perhaps limit to 3 or 6 sig. fig.?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not much for task node since we don't have too many of them, that's why i didn't add the rounding in the first place here. It did make a big difference for task instance node since we have lots of them, IIRC, probably around 10-20% size reduction.
I can change it to round to 3 sig. fig. everywhere to see what the performance implication would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashb round to 3 sig. fig. increases the overall payload size by 15%. The question now is do we care about millisecond accuracy for task start/end time enough to take this 15% performance hit?
So far, I found second granularity has been good enough for us, but I might be missing other use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not needed.
Is it worth making it a config option do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code path has a very hot function call loop that's very sensitive to if statements. For the large DAG that we have, adding one extra if statement increases the response time by more than 400ms. That's why
simplify reduce_nodes() logic, remove unnecessary if statements
is in the optimization list :)That and based on the understanding that we are rewriting Airflow web into a proper SPA, I think it's best not to introduce a config for this change. I would prefer us giving round to second a try and come back to add more sig. fig. or add a config later on if any real use-case comes up. It's better to not engineer solutions when we don't have a good use-case in mind.
If you are really concerned about the precision, we can perhaps change it to round to 1 sig. fig. for a 7% performance hit. I can't really think of a case where knowing 0.01 second difference of runtime is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good reasoning. I'll copy some/most of this in to the commit message for future-proofing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this unresolved as a reminder to myself.