Skip to content

Rename TransactionTimeout to more descriptive LockWaitTimeout#31223

Merged
kamipo merged 1 commit intorails:masterfrom
kamipo:rename_to_lock_wait_timeout_exception
Nov 27, 2017
Merged

Rename TransactionTimeout to more descriptive LockWaitTimeout#31223
kamipo merged 1 commit intorails:masterfrom
kamipo:rename_to_lock_wait_timeout_exception

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented Nov 25, 2017

Since #31129, new error class StatementTimeout has been added.
TransactionTimeout is caused by the timeout shorter than
StatementTimeout, but its name is too generic. I think that it should
be a name that understands the difference with StatementTimeout.

Since rails#31129, new error class `StatementTimeout` has been added.
`TransactionTimeout` is caused by the timeout shorter than
`StatementTimeout`, but its name is too generic. I think that it should
be a name that understands the difference with `StatementTimeout`.
@matthewd
Copy link
Member

👍

The previous names make it sound like one should inherit from the other, but they're really not related enough for that to make sense. Clarifying the names seems like a good idea.

On a similar note, should we consider StatementTimeout -> QueryAborted or something? I don't know about MySQL's ER_QUERY_TIMEOUT, but I believe the Postgres error code is also used when the query is manually cancelled by an administrator... something that doesn't quite so explicitly blame a timeout might be a good idea?

Copy link
Contributor

@albertoalmagro albertoalmagro left a comment

Choose a reason for hiding this comment

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

👏

@kamipo
Copy link
Member Author

kamipo commented Nov 27, 2017

I created new PR #31235 for canceling statement due to user request.

@kamipo kamipo merged commit ad0630f into rails:master Nov 27, 2017
@kamipo kamipo deleted the rename_to_lock_wait_timeout_exception branch November 27, 2017 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants