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 a session backend to store session data in the database #21167

Closed
wants to merge 5 commits into from

Conversation

blag
Copy link
Contributor

@blag blag commented Jan 27, 2022

This PR re-implements modified portions of Flask-Session to store session data in the database.

Things to note:

  • This is my first major PR
  • I'm not sure the added test file is in the right place
  • Please double check my migration documentation is acceptable and complete

TODO:

  • Tests

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jan 27, 2022
@blag blag force-pushed the db-session-backend branch 2 times, most recently from 38a3cc6 to 782de86 Compare January 28, 2022 05:39
@blag blag marked this pull request as ready for review January 28, 2022 05:40
@apache apache deleted a comment from potiuk Jan 28, 2022
@blag blag force-pushed the db-session-backend branch from 0017000 to 34245ed Compare January 28, 2022 22:27
ashb
ashb previously requested changes Jan 31, 2022
airflow/models/__init__.py Outdated Show resolved Hide resolved
@jedcunningham
Copy link
Member

We need to add it to NOTICE as well.

@blag blag force-pushed the db-session-backend branch 3 times, most recently from cc6b1ad to ad88d10 Compare February 1, 2022 18:10
@blag blag requested a review from mik-laj as a code owner February 1, 2022 18:10
airflow/_vendor/flask_session/sessions.py Outdated Show resolved Hide resolved
airflow/_vendor/flask_session/sessions.py Outdated Show resolved Hide resolved
airflow/www/extensions/init_session.py Outdated Show resolved Hide resolved
airflow/www/extensions/init_session.py Outdated Show resolved Hide resolved
airflow/www/extensions/init_session.py Show resolved Hide resolved
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
@@ -23,7 +23,9 @@ Here's the list of all the Database Migrations that are executed via when you ru
+--------------------------------+------------------+-----------------+---------------------------------------------------------------------------------------+
| Revision ID | Revises ID | Airflow Version | Description |
+--------------------------------+------------------+-----------------+---------------------------------------------------------------------------------------+
| ``e655c0453f75`` (head) | ``587bdf053233`` | ``2.3.0`` | Add ``map_index`` column to TaskInstance to identify task-mapping, and a ``task_map`` |
| ``c381b21cb7e4`` (head) | ``e655c0453f75`` | ``2.3.0`` | Create a ``sessions`` table to store web session data |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be moved into the right spot for 2.2.4 (e.g. a different "revises id"), but I'm most likely going to pull another revision back to 2.2.4 tomorrow so hold off for now. At least this comment will remind us we need to do it.

airflow/_vendor/flask_session/db_models.py Outdated Show resolved Hide resolved
@blag
Copy link
Contributor Author

blag commented Feb 2, 2022

@jedcunningham Some of the code your wanted changes to is the vendored in code. I intentionally made as minimal modifications to that code as I needed, with the intent to possibly make a PR to upstream Flask-Session, get that merged, and migrate to using that (and removing our vendored code). The more and more in-depth modifications we make to the vendored code, the more difficult it will be to switch in the future.

I'm happy to make the changes you noted if you'd still like me to (please let me know), I just wanted to bring that to your attention first.

@blag blag force-pushed the db-session-backend branch 5 times, most recently from 1d40916 to 8ced244 Compare February 5, 2022 02:03
@dstandish
Copy link
Contributor

dstandish commented Feb 7, 2022

@jedcunningham Some of the code your wanted changes to is the vendored in code. I intentionally made as minimal modifications to that code as I needed, with the intent to possibly make a PR to upstream Flask-Session, get that merged, and migrate to using that (and removing our vendored code). The more and more in-depth modifications we make to the vendored code, the more difficult it will be to switch in the future.
I'm happy to make the changes you noted if you'd still like me to (please let me know), I just wanted to bring that to your attention first.

On this topic, thinking out loud...

If we're trying to be able to easily understand what changes we've made to the vendored code, should we merge it in first with no changes? then make PRs on top of that which modify its behavior?
Then we'd be able to tell exactly what we did to it (instead of vendoring and changing in one commit / pr).
And it would make the PRs easier to review too.

Alternatively I don't think we do submodules 'round here but is this a valid case for it?

@ashb
Copy link
Member

ashb commented Feb 7, 2022

If we're trying to be able to easily understand what changes we've made to the vendored code, should we merge it in first with no changes? then make PRs on top of that which modify its behavior?

In a case or two previous to this we have merged the PR without squashing (manually on the command line).

@ashb ashb dismissed their stale review February 7, 2022 18:53

Outdated

@blag
Copy link
Contributor Author

blag commented Feb 7, 2022

@dstandish That's a good call, but it's looking less and less likely that our changes are going to be palatable to the maintainer of Flask-Session. If the changes we need won't ever be merged into Flask-Session, then our modified vendored code just becomes...our code, with an additional copyright line/header.

A "middle path" of this would be to start maintaining our own fork of Flask-Session, merge in the changes that we need, maybe merge in a few more unmerged PRs from Flask-Session, and have our fork become the friendlier (than Flask-Session) session package for Flask. But I don't want to sign the company up for that responsibility without some consensus and blessing, and I don't really want to maintain my own personal fork of Flask-Session specifically for Airflow.

I'm happy to do whatever it takes to get this merged, but I will need some guidance on how to get there.

@blag blag force-pushed the db-session-backend branch 2 times, most recently from 35937be to ef3fa62 Compare February 8, 2022 18:16
@jedcunningham
Copy link
Member

Superseded by #21478.

@ashb ashb deleted the db-session-backend branch February 15, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants