-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks hacky. Just wondering if there is a better way to handle this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that DB errors should not be exposed to controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will you handle similar errors for other api calls? eg: Alreadyexists for RegisterExperiment etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
/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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
WDYT @johnugeorge
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