vterrors: addres review comments #2593
Conversation
|
I'll let Michael review this. |
| // ErrorStats shows number of critial erros happened. | ||
| ErrorStats = stats.NewCounters("Errors", "Fail", "TxPoolFull", "NotInTx", "Deadlock", "Fatal") | ||
| ErrorStats = stats.NewCounters( | ||
| "Errors", |
There was a problem hiding this comment.
What is "Errors"? A total of all values?
In general, it's not advised to have a total in your map as well. Instead, the monitoring system should figure this out for you. Maybe ask Javeen on his opinion on this?
There was a problem hiding this comment.
NewCounters is actually a misleading constructor. Errors is the main tag, and the rest are sub-categories.
| ErrorStats = stats.NewCounters("Errors", "Fail", "TxPoolFull", "NotInTx", "Deadlock", "Fatal") | ||
| ErrorStats = stats.NewCounters( | ||
| "Errors", | ||
| "OK", |
There was a problem hiding this comment.
Can you please use the enums here instead?
(I understand that we're probably not going to change the names. But still, this way the connection is more clear where this list comes from :)
There was a problem hiding this comment.
You mean vtrpcpb.Code_OK.String()?
go/vt/vterrors/aggregate.go
Outdated
| @@ -25,10 +25,10 @@ const ( | |||
| PriorityOutOfRange | |||
| // Potentially retryable errors. | |||
| PriorityUnavailable | |||
| PriorityFailedPrecondition | |||
| PriorityResourceExhausted | |||
There was a problem hiding this comment.
ResourceExhausted should go into the "permanent errors" section.
go/vt/vterrors/doc.go
Outdated
| Vitess also defines a VitessError error implementation, that can convert | ||
| any error and add a code to it. | ||
| Vitess also defines a standardized error implementation that allows | ||
| you to build an error with an associated cannonical code. |
Addressed review comments. I've changed the error codes for cases we've agreed on. I've left the more contentious ones unchanged for now. BUG=32851872
|
LGTM |
Addressed review comments. I've changed the error codes for
cases we've agreed on. I've left the more contentious ones
unchanged for now.
BUG=32851872