Centralises JobsAPI errors and gRPC error codes#8346
Conversation
jake-engelberg
left a comment
jake-engelberg
left a comment
There was a problem hiding this comment.
I don't see any non-testing usage of NotFound(), NotConfigured(), or IncorrectNegative().
Should we remove them to reduce confusion upon looking through this file?
Signed-off-by: Elena Kolevska <[email protected]>
43d0fd5 to
1fdb021
Compare
Signed-off-by: Elena Kolevska <[email protected]>
Yeah, they're not used. Let's delete them, since we're here. 👍 |
| const ( | ||
| InFixName ReasonSegment = "NAME_" | ||
| InFixNegative ReasonSegment = "NEGATIVE_" | ||
| PostFixName ReasonSegment = "NAME" | ||
| PostFixEmpty ReasonSegment = "EMPTY" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Deleting because they're not used anywhere.
| Build() | ||
| } | ||
|
|
||
| func NotConfigured(name string, componentType string, metadata map[string]string, grpcCode codes.Code, httpCode int, legacyTag string, reason string, category errorcodes.Category) error { |
There was a problem hiding this comment.
Deleting because not used anywhere.
| Build() | ||
| } | ||
|
|
||
| func IncorrectNegative(name string, metadata map[string]string, reason string, category errorcodes.Category) error { |
There was a problem hiding this comment.
Deleting because not used anywhere.
| const ( | ||
| CodePrefixScheduler = "SCHEDULER_" // TODO(Cassie): move this to kit eventually | ||
| InFixJob ReasonSegment = "JOB_" | ||
| InFixAppID ReasonSegment = "APPID_" | ||
| InFixGet ReasonSegment = "GET_" | ||
| InFixList ReasonSegment = "LIST_" | ||
| InFixDelete ReasonSegment = "DELETE_" | ||
| InFixSchedule ReasonSegment = "SCHEDULE_" | ||
| PostFixRepeats ReasonSegment = "REPEATS" | ||
| PostFixJob ReasonSegment = "JOB" | ||
| PostFixJobs ReasonSegment = "JOBS" | ||
|
|
||
| MsgScheduleJob = "failed to schedule job" | ||
| MsgGetJob = "failed to get job" | ||
| MsgListJobs = "failed to list jobs" | ||
| MsgDeleteJob = "failed to delete job" | ||
| ) |
There was a problem hiding this comment.
Moved to pkg/messages/errorcodes/errorcodes.go to align with other error codes
Signed-off-by: Elena Kolevska <[email protected]>
elena-kolevska
changed the title
|
FYI @msfussell |
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]>
|
@jake-engelberg We'll have to update this once your PR gets merged. |
| CommonMalformedResponse = ErrorCode{"ERR_MALFORMED_RESPONSE", "", CategoryCommon} // Malformed response | ||
|
|
||
| // ### Scheduler/Jobs API | ||
| SchedulerScheduleJob = ErrorCode{"SCHEDULER_SCHEDULE_JOB", "SCHEDULER_SCHEDULE_JOB", CategoryJob} // Error scheduling job |
There was a problem hiding this comment.
I think this is what you are wanting to prefix DAPR_ with - correct?
| SchedulerScheduleJob = ErrorCode{"SCHEDULER_SCHEDULE_JOB", "SCHEDULER_SCHEDULE_JOB", CategoryJob} // Error scheduling job | |
| SchedulerScheduleJob = ErrorCode{"SCHEDULER_SCHEDULE_JOB", "DAPR_SCHEDULER_SCHEDULE_JOB", CategoryJob} // Error scheduling job |
There was a problem hiding this comment.
Yeah, exactly, this requirement comes from the error codes proposal: https://github.com/dapr/proposals/blob/main/20230511-BCIRS-error-handling-codes.md#errorinfo-required.
It's an easy change here, but we need to check if we're still on time to make a change in the sdks, if needed.
There was a problem hiding this comment.
We should probably add a string check to kit to check if DAPR_ is prefixed and if not to add it to avoid this going forward. This was missed bc the const lives in runtime atm, but all the others live in kit and are prefixed in kit. I think I assumed that prefix was added to the const when I added it to runtime
There was a problem hiding this comment.
Since scheduler isn't stable 'yet', my vote would be update this to include the dapr prefix. Elena, could you go ahead with this since you have this PR open?
There was a problem hiding this comment.
Yes, definitely! Now is the right time to change this.
Signed-off-by: Elena Kolevska <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]> # Conflicts: # pkg/messages/errorcodes/errorcodes.go
Signed-off-by: Elena Kolevska <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
5 participants
We're in the process of centralising all errors and error codes as much as possible for easier metrics, documentation and general code simplicity.
This PR centralises all the error codes for the Jobs API in a single file where all the errors are defined for easy parsing and docs generation.
In addition, we're adding a gRPC code field to the ErrorCode struct, so we can track all the possible gRPC errors as well.
Task list:
This is how an autogenerated table would look like. No value in the gRPC code field means we don't have a rich gRPC error for that code yet.
