Skip to content
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

Add description to pools view #34862

Merged
merged 3 commits into from Oct 20, 2023

Conversation

vchiapaikeo
Copy link
Contributor

@vchiapaikeo vchiapaikeo commented Oct 11, 2023

closes: 34751

Expose description column in the pools view

Before:
image

After:
image

Not sure if it's okay to put styles inline. If I can refactor to a css file, please let me know.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Oct 11, 2023
@vchiapaikeo vchiapaikeo marked this pull request as ready for review October 11, 2023 09:27
@uranusjr
Copy link
Member

I feel we should trim the string a bit, the description can be extremely long.

@vchiapaikeo
Copy link
Contributor Author

Good call. What about some additional styling?

image image

@uranusjr
Copy link
Member

Is it possible to also do truncation when the rows are selected? Loading a long value into memory can be slow itself and bring down the entire view. I know it can be done with substr but the FAB model adds a layer on it.

@eladkal
Copy link
Contributor

eladkal commented Oct 12, 2023

I feel we should trim the string a bit, the description can be extremely long.

What do we do in the Variable page? We have description column there as well.
@vchiapaikeo maybe we already have it handled there and we can just copy it?

@vchiapaikeo
Copy link
Contributor Author

vchiapaikeo commented Oct 13, 2023

Ah yes, VariableModelView uses a custom FAB Model list_template and injects additional styling there. We can do something similar to get column resizing in PoolModelView:

image

However, to @uranusjr's point, we are still loading a potentially long value into memory. @uranusjr , when you say do truncation when the rows are selected, what do you think about adding a custom formatter and doing the slicing there?

@uranusjr
Copy link
Member

No, I’m thinking if it’d be posible to apply substr on the column when the rows are selected from the database. That’s where the long strings get loaded. When you reach the renderer, the strings are already in memory and would already cause load on the server; a custom renderer is still better than only CSS (some bytes saved transfering HTML) but not too much different at that point.

@vchiapaikeo
Copy link
Contributor Author

Ah I see. Wasn't sure if you were referring to browser memory or server memory. I profiled the render template code, and I'm not seeing an obvious place to add this. Are you thinking of something more custom on the sqlalchemy model? column_property or @hybrid_property come to mind but may be a bit overkill.

@vchiapaikeo
Copy link
Contributor Author

@uranusjr , what are your thoughts on the previous comment? Sorry - forgot to tag ya.

@uranusjr
Copy link
Member

Yeah I was wondering if Flask Appbuilder has a solution to this without needing to add something to the model. In Django admin you can add a method on the view class and point a column to it, for example. But I wouldn’t be surprised if there’s nothing similar; F.A.B. is not particularly known for being flexible… If you don’t see anything straightforward I can live with just adding custom styling like Variable for now and wait for people to complain (if they ever do).

@vchiapaikeo
Copy link
Contributor Author

Did a bit of research on FAB and am not seeing anything obvious that would allow us to modify the underlying query being executed against the model. Rebasing and I think this should be good to go provided tests continue to pass.

@eladkal eladkal added this to the Airflow 2.8.0 milestone Oct 17, 2023
@eladkal eladkal added the type:improvement Changelog: Improvements label Oct 17, 2023
@eladkal eladkal merged commit 0583150 into apache:main Oct 20, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose description columns of Pools in the UI
4 participants