aminghadersohi commented on code in PR #44091:
URL: https://github.com/apache/superset/pull/44091#discussion_r4028774758
##########
superset/datasets/api.py:
##########
@@ -796,6 +867,16 @@ def put(self, pk: int) -> Response:
)
response = self.response_422(message=str(ex))
except DatasetUpdateFailedError as ex:
+ # The gap lock the conditional path's locking read takes on a
+ # zero-live-row range can deadlock against another conditional
+ # writer at Continuum's version-row INSERT inside the command;
+ # on_error chains the driver error as __cause__, and the update
+ # transaction has rolled back. (The post-commit override_columns
+ # refresh raises its own exception type and cannot reach this
+ # branch.) Same retryable classification as the read-point
+ # handler above: the token is not proven stale.
+ if conditional and is_lock_contention_error(ex.__cause__):
Review Comment:
Only negative coverage here: the contaminated-`DataError` test asserts this
does NOT fire. Replacing it with `if False:` leaves all 52 unit tests green. A
positive twin — `args=(1213, ...)` as `__cause__` — passes as written and fails
once removed. Credits fitzee's 422-vs-409 thread.
##########
superset/datasets/api.py:
##########
@@ -571,6 +573,25 @@ def post(self) -> Response:
logger.exception("Unexpected error in DatasetRestApi.post")
return self.response_500(message="Fatal error")
+ def _lock_contention_response(self) -> Response:
+ """The shared retryable 409 for a conditional save losing a lock race.
+
+ The rollback is LOAD-BEARING for lock-wait-timeout: with
+ ``innodb_rollback_on_timeout`` OFF (the MySQL default) a 1205
+ rolls back only the failing STATEMENT -- the transaction is still
+ alive and still holds any entity row lock already acquired, and
+ this rollback is what releases it. For a deadlock (1213) InnoDB
+ already rolled the transaction back and this clears the aborted
+ session before responding.
+ """
+ db.session.rollback() # pylint: disable=consider-using-transaction
Review Comment:
`db.session.rollback()` → `pass` also survives all 52 tests. fitzee's thread
warned a future cleanup trusting the comment would leak the entity lock on a
1205; the docstring now says LOAD-BEARING, but nothing pins it. Asserting it on
the 1205 path closes that.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
