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
Alt: Add a session backend to store session data in the database #21478
Alt: Add a session backend to store session data in the database #21478
Conversation
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
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.
LGTM
We can drop the table like this without needing a model |
|
||
def upgrade(): | ||
"""Apply add session table to db""" | ||
op.create_table( |
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.
I think we should also add an ORM model for this table so that it's not only through migration file that the table can be created.
I'm working on a change that would have new DBs created through the ORM rather than going through the migration files. #21462
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.
We can do it like this I think
from flask.session import SqlAlchemySessionInterface
from flask_appbuilder import SQLA
...
db = SQLA()
db.session = settings.Session
SessionModel = SqlAlchemySessionInterface(db=db).sql_session_model
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.
(If we should or not is the next question)
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 is what it ends up looking like, when considering #21462's changes:
--- a/airflow/utils/db.py
+++ b/airflow/utils/db.py
@@ -606,12 +606,21 @@ def initdb(session: Session = NEW_SESSION):
from airflow.models.base import Base
from airflow.www.app import create_app
+ from airflow.www.session import AirflowDatabaseSessionInterface
+ from flask_sqlalchemy import SQLAlchemy
+
Base.metadata.create_all(settings.engine)
# Stamp migration head with alembic
config = _get_alembic_config()
command.stamp(config, "head")
# sets up the db; Sync permissions etc
- create_app(config={'UPDATE_FAB_PERMS': True})
+ app = create_app(config={'UPDATE_FAB_PERMS': True})
+
+ # Create `session` table
+ db = SQLAlchemy(app)
+ AirflowDatabaseSessionInterface(app=app, db=db, table='session', key_prefix='')
+ db.create_all()
+
if conf.getboolean('core', 'LOAD_DEFAULT_CONNECTIONS'):
create_default_connections(session=session)
c44e641
to
85c7b9a
Compare
fbd11aa
to
b0d1e05
Compare
b0d1e05
to
9b16f00
Compare
Co-authored-by: Jed Cunningham <jedcunningham@apache.org> (cherry picked from commit da9d086)
Co-authored-by: Jed Cunningham <jedcunningham@apache.org> (cherry picked from commit da9d086)
This is an alternate approach to #21167, the primary difference being we don't need to vendor in Flask-Session.
The downside, however, is the weirdness of having our own session model that isn't actually used other than to remove the table duringdb reset
. Is there a better way to accomplish that?