aggregate err when checking options #3586
Conversation
volcano-sh-bot
added
the
kind/feature
volcano-sh-bot
added
the
size/S
|
/assign @Monokaix @lowang-bh |
|
/PTAL thanks! |
| // check controllers option | ||
| var allErrors []error | ||
|
|
||
| // Check controllers option | ||
| if err := s.checkControllers(); err != nil { |
There was a problem hiding this comment.
Before this pr, err will return directly once checkControllers has err, but after the change, other err will also be checked, which cause more code be executed, is this reasonable?
There was a problem hiding this comment.
IMO, this makes sense, because we are checking the options. The options should be checked as a whole and throw all errors, rather than throwing errors at each step.
There was a problem hiding this comment.
How about k8s's implements? Some time only the first error is need to be paid attention to.
There was a problem hiding this comment.
We can refer to the implementation of kube-scheduler. The configuration is returned only after the entire verification is completed. The same goes for other components
googs1025
force-pushed
the
aggregate_err
branch
2 times, most recently
from
b25a22f
to
3f48a7a
Compare
Signed-off-by: googs1025 <[email protected]>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Monokaix 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 |
volcano-sh-bot
added
the
approved
/kind feature