Rename MySQL error to use a more generic name #1619
Conversation
Eragonfr
force-pushed
the
rename-mysql-generic
branch
from
ea2dfaa
to
0fd7740
Compare
|
For additional context: eragonfr is working on the sqlite backend for syncstorage. One of the refactoring changes she would like to make is to qualify the I'd like to get y'all's opinion as well. |
| _ => true, | ||
| } | ||
| } | ||
|
|
||
| fn metric_label(&self) -> Option<String> { | ||
| Some( | ||
| match self.kind { | ||
| MysqlErrorKind::DieselQuery(_) => "storage.mysql.error.diesel_query", |
There was a problem hiding this comment.
These metric changes do not impact our dashboards.
Eragonfr
force-pushed
the
rename-mysql-generic
branch
from
d44f7c4
to
21055cb
Compare
|
I'll poke the other two to get another review before I land this. (Sorry, things have been a bit chaotic over here.) |
| @@ -5,17 +5,17 @@ use http::StatusCode; | |||
| use syncserver_common::{from_error, impl_fmt_display, ReportableError}; | |||
| use thiserror::Error; | |||
|
|
|||
| /// Error specific to any MySQL database backend. These errors are not related to the syncstorage | |||
| /// Error specific to any SQL database backend. These errors are not related to the syncstorage | |||
| /// or tokenserver application logic; rather, they are lower-level errors arising from diesel. | |||
There was a problem hiding this comment.
This comment still stands, "lower-level errors arising from diesel" and I think suggests a clearer name for this: DieselError, how does that sound?
There was a problem hiding this comment.
That seems like a good name too. As all errors are from diesel or it's dependencies (r2d2 or diesel_migrations).
But if we use SqlError that means that we could replace diesel with an other ORM.
If someone wants to support a database that's not supported by Diesel. But maybe in that case they'll need to do something similar to the spanner backend.
Idk. If @jrconlin and @taddes prefer DieselError I can change it.
There was a problem hiding this comment.
Honestly, I'm fine either way, but I do appreciate the concept of having a more generic SqlError for the reason that @Eragonfr notes. I don't really see us replacing diesel any time soon, but the sources of the error being reported are coming from the underlying SQL system. DieselError type should be reserved for errors arising from the diesel system directly (e.g. failure to connect to the data store, or some configuration break).
Eragonfr
force-pushed
the
rename-mysql-generic
branch
from
21055cb
to
07a816a
Compare
Description
The MysqlError and related types can be reused by other SQL based backends. See PR #1520
Testing
Test that everything still works like on master
Issue(s)
Not really an issue but here's the review about splitting this from #1520