Implement GetExperimentInDB #558
Conversation
|
/hold |
|
/hold cancel |
| @@ -140,8 +141,13 @@ func (g *DefaultValidator) validateSupportedJob(job *unstructured.Unstructured) | |||
| } | |||
|
|
|||
| func (g *DefaultValidator) validateForCreate(inst *experimentsv1alpha2.Experiment) error { | |||
| isErrNoRows := func (e error) bool { | |||
There was a problem hiding this comment.
Why should we use such a func instead of sql.ErrNoRows, is there any problem that we want to avoid?
There was a problem hiding this comment.
sql.ErrNoRows is raised by db interface in grpc server, in grpc client side, we cannot get sql.ErrNoRows, which has been wrapped.
There was a problem hiding this comment.
It looks hacky. Just wondering if there is a better way to handle this
There was a problem hiding this comment.
I feel that DB errors should not be exposed to controller
There was a problem hiding this comment.
Could we catch the error in GetExperimentFromDB? If the error is sql.ErrNoRows we return nil, or we return error to the controller.
There was a problem hiding this comment.
how will you handle similar errors for other api calls? eg: Alreadyexists for RegisterExperiment etc
There was a problem hiding this comment.
For others APIs, our use cases don't need care what the error type is now when error is not nil.
There was a problem hiding this comment.
Can we update managerclient.go as follows:
func (d *DefaultClient) GetExperimentFromDB(instance *experimentsv1alpha2.Experiment) (*api_pb.GetExperimentReply, error) {
request := &api_pb.GetExperimentRequest{
ExperimentName: instance.Name,
}
if exp, err := commonv1alpha2.GetExperiment(request); err != nil {
if !isErrNoRows(err) {
return nil, fmt.Errorf("Fail to check record for the experiment in DB: %v", err)
}
}
return exp, nil
}Then the error will be handled in manager client and not be exposed to the controller.
There was a problem hiding this comment.
I had used "PreCheckRegisterExperiment" to replace "GetExperimentFromDB" in latest commit.
I think we should avoid to parse Error message string to judge what its original error in grpc server side is, especially when we support more DB backends, we must refactor isErrNoRows to parse the error message string to match other backends ErrNoRows counterparts.
|
/test coverage/coveralls |
|
/retest |
|
@richardsliu Could we allow the decrease of coveralls? We can set a floor for it. Maybe 50% |
|
@gaocegege We can set a failure threshold at 50%, and a coverage decrease threshold at maybe 5%. Does that sound ok? |
|
SGTM, thanks. |
|
@gaocegege Done. |
|
@hougangliu Please rebase |
|
/test Travis CI |
|
LGTM In general, i feel that the right solution is to have a translation layer between DB and GRPC to translate the DB errors to common set of errors defined for GRPC that can be used by any caller(eg: controller, api user, UI etc). I am sure that there will be more errors to be differentiated at the caller level( controller here) and this solution might not scale. |
For now, the reason why we didn't do the translation is that so far we don't care what the error exactly is except this validation case. So I don't think it's worth spending much time to add a translation layer just for hiding backend details, at least, its priority is not high. Anyway, we can create a issue to trace discuss for it so that we may do it in coming release |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hougangliu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change is