Rename mimirpb.INVALID error cause into mimirpb.UNKNOWN_CAUSE #6493
Conversation
Signed-off-by: Yuri Nikolic <[email protected]>
pkg/ingester/errors_test.go
Outdated
| @@ -709,3 +709,43 @@ func checkErrorWithStatusDetails(t *testing.T, details []any, expectedDetails *m | |||
| require.Equal(t, expectedDetails, errDetails) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: Feels a bit odd to put methods from a non-test type into the _test.go file. Can you add a comment explaining that this is only needed for tests?
There was a problem hiding this comment.
I have reverted this change, and put the methods back to errors.go with a comment stating that these methods are needed for tests only.
Signed-off-by: Yuri Nikolic <[email protected]>
* Rename mimirpb.INVALID error cause into mimirpb.UNKNOWN_CAUSE Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]>
|
I can't agree with this change.
The reason why I suggested to have an With this new version of the code it is unclear when the code is buggy and it didn't set the reason, and when the error was processed and it falls into the "unknown" case. I'm also not sure when the cause is "Unknown". It seems that we set this into the details of "Unavailable" errors, but maybe in those cases we just shouldn't set any details at all? |
What this PR does
This PR renames the
mimirpb.INVALIDsymbolic value of typemimirpb.ErrorCauseintroduced in #6377 intomimirpb.UNKNOWN_CAUSE. The reason for this is thatmimirpb.INVALIDcould be misinterpreted as something that is not valid, but its actual purpose is to represent an unknown error cause.Which issue(s) this PR fixes or relates to
Relates to #6008
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]