Refine job errors #839
Refine job errors #839
Conversation
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/job
branch
from
ae96521 to
1f7a092
Compare
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/vc
branch
from
515aa90 to
9a81cda
Compare
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/job
branch
from
1f7a092 to
7d09142
Compare
| error: 'JobNotFound', | ||
| message: `could not find job ${jobName}`, | ||
| }); | ||
| return next(error); |
There was a problem hiding this comment.
Since this error has code property in this branch, assume that it has been already transformed into HttpError in utils/error.js
| } else if (jobList === undefined) { | ||
| // Unreachable |
There was a problem hiding this comment.
If these lines are unreachable, why do we still preserve them?
There was a problem hiding this comment.
@Gerhut only refined error handling in this PR.
| message: err.message, | ||
| }); | ||
| } | ||
| return next(createError.unknown(err)); |
There was a problem hiding this comment.
I think your method covers too many details.
There was a problem hiding this comment.
I suggest we split errors into two categories:
- Internal errors, which are raised by models and handled by controllers;
- Exposed errors, which will be returned to API callers.
Errors in the first category contain many infrastructure-specific details, for example HDFS error, launcher error, VC error, Yarn error, token error, etc., which are not necessary to be exposed to API callers. After the processed by controllers, these internal errors will eventually be reorganized and translated into exposed errors, which contain meaningful information relevant to the knowledge of external developers, who care more about higher-level concepts such as input syntax, storage, computational resource status, authentication, etc.
There was a problem hiding this comment.
I think those errors have been handled in models instead of here.
There was a problem hiding this comment.
VirtualClusterNotFoundraised in https://github.com/Microsoft/pai/blob/qixcheng/rest-server-refine-error/job/rest-server/src/models/user.js#L204NoRightAccessVirtualClusterraised in https://github.com/Microsoft/pai/blob/qixcheng/rest-server-refine-error/job/rest-server/src/models/user.js#L216VirtualClusterNotFoundInDatabasecovers too many details in https://github.com/Microsoft/pai/blob/qixcheng/rest-server-refine-error/job/rest-server/src/models/user.js#L208 so I throwed aUnknownErrorwith HDFS Error message.
rest-server/src/models/job.js
Outdated
| @@ -119,9 +123,9 @@ class Job { | |||
| if (requestRes.status === 200) { | |||
| next(this.generateJobDetail(requestResJson), null); | |||
| } else if (requestRes.status === 404) { | |||
| next(null, new Error('JobNotFound')); | |||
| next(null, createError('Not Found', 'ERR_NO_JOB', `Job ${name} not found.`)); | |||
There was a problem hiding this comment.
move error to the first parameter?
| message: err.message, | ||
| }); | ||
| } | ||
| return next(createError.unknown(err)); |
There was a problem hiding this comment.
I think those errors have been handled in models instead of here.
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/vc
branch
from
df3f1d9 to
06578ef
Compare
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/job
branch
from
1bbdb47 to
4a4b087
Compare
Gerhut
force-pushed
the
qixcheng/rest-server-refine-error/job
branch
from
4a4b087 to
1c05d72
Compare
* Refine job errors * Add test cases * REST Server: Rename error codes according to the Mirosoft api-guideline
* Refine job errors * Add test cases * REST Server: Rename error codes according to the Mirosoft api-guideline
#807