You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/11/16 20:32:50 UTC
[PR] chore: Remove autoflush [superset]
john-bodley opened a new pull request, #26009:
URL: https://github.com/apache/superset/pull/26009
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26009:
URL: https://github.com/apache/superset/pull/26009#issuecomment-1815567125
@villebro in terms of next steps, today's discussion (in conjunction with this PR) helped to solidify what is viable with SQLAlchemy event handlers and thus I updated SIP-99B with regards to how this should be handled moving forward. The TL;DR is ideally we should move said logic either to the database (if said events previously were handling logic which could be captured via a `CASCADE` constraint) or Command.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #26009:
URL: https://github.com/apache/superset/pull/26009
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #26009:
URL: https://github.com/apache/superset/pull/26009#issuecomment-1815556595
> @villebro I _think_ the error you might have been seeing, per what I was witnessing earlier when trying the `session = inspect(target).session` approach, was actually,
>
> > This transaction is closed
>
> i.e., it was transaction as opposed to session related. The reason being (as mentioned in Slack) was that in the SQLAlchemy [event documentation](https://docs.sqlalchemy.org/en/20/orm/events.html#mapper-events) it states:
>
> > Mapper-level flush events only allow **very limited operations**, on attributes local to the row being operated upon only, as well as allowing any SQL to be emitted on the given `Connection`.
>
> So when trying to leverage the Flask-SQLAlchemy session—via either `inspect(target).session` or `db.session`—the associated transaction was closed because said code was trying to perform operations outside of operations limited to the row (`target`) being operated on, i.e., the `after_insert` event listener is associated with a chart and we then try to create an associated tag. Creating a new session from the connection results in a new transaction for us to emit any SQL operation (per the SQLAlchemy statement).
>
> Note in [[SIP-99B] Proposal for (re)defining a "unit of work"](https://github.com/apache/superset/issues/25108) this type of operation will not be allowed because it violates the "unit of work".
@john-bodley got it, this is all finally starting to make sense. Thanks for taking this on - once finished, this will have a big positive impact on the backend code quality, performance, maintainability etc..
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26009:
URL: https://github.com/apache/superset/pull/26009#discussion_r1396475462
##########
superset/tags/models.py:
##########
@@ -111,7 +111,7 @@ class TaggedObject(Model, AuditMixinNullable):
tag = relationship("Tag", back_populates="objects", overlaps="tags")
-def get_tag(name: str, session: Session, type_: TagType) -> Tag:
+def get_tag(name: str, session: orm.Session, type_: TagType) -> Tag:
Review Comment:
We redefine the `Session` class on line 38 so the previous type hint wasn't completely accurate.
##########
superset/tags/models.py:
##########
@@ -166,9 +166,7 @@ def after_insert(
connection: Connection,
target: Dashboard | FavStar | Slice | Query | SqlaTable,
) -> None:
- session = Session(bind=connection)
-
- try:
+ with Session(bind=connection) as session:
Review Comment:
Opting for a context manager which closes the session on exit.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26009:
URL: https://github.com/apache/superset/pull/26009#issuecomment-1815542275
@villebro I _think_ the error you might have been seeing, per what I was witnessing earlier when trying the `session = inspect(target).session` approach, was actually,
> This transaction is closed
i.e., it was transaction as opposed to session related. The reason being (as mentioned in Slack) was that in the SQLAlchemy [event documentation](https://docs.sqlalchemy.org/en/20/orm/events.html#mapper-events) it states:
> Mapper-level flush events only allow **very limited operations**, on attributes local to the row being operated upon only, as well as allowing any SQL to be emitted on the given `Connection`.
So when trying to leverage the Flask-SQLAlchemy session—via either `inspect(target).session` or `db.session`—the associated transaction was closed because said code was trying to perform operations outside of operations limited to the row (`target`) being operated on, i.e., the `after_insert` event listener is associated with a chart and we then try to create an associated tag. Creating a new session from the connection results in a new transaction for us to emit any SQL operation (per the SQLAlchemy statement).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: Remove unnecessary autoflush from tagging and key/value workflows [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #26009:
URL: https://github.com/apache/superset/pull/26009#issuecomment-1815530179
@john-bodley so those closed sessions in the event handlers were caused by those explicit `finally: session.close()` calls?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org