-
Notifications
You must be signed in to change notification settings - Fork 489
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
[ISSUE #52] Remove noise error log message #54
Conversation
Signed-off-by: huanwei <huan@harmonycloud.cn>
PTAL. @weekface |
The zero value comparison in this case should be the empty string rather than nil. |
pkg/util/util.go
Outdated
glog.Errorf("failed to parse CPU resource %s to quantity: %v", spec.Requests.CPU, err) | ||
} else { | ||
rr.Requests[corev1.ResourceCPU] = q | ||
if spec.Requests.CPU != 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.
spec.Requests.CPU
's type is string
, can't compared with nil
, replace nil
with ""
instead
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.
Got.
pkg/util/util.go
Outdated
glog.Errorf("failed to parse memory resource %s to quantity: %v", spec.Requests.Memory, err) | ||
} else { | ||
rr.Requests[corev1.ResourceMemory] = q | ||
if spec.Requests.Memory != 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.
ditto
pkg/util/util.go
Outdated
glog.Errorf("failed to parse CPU resource %s to quantity: %v", spec.Limits.CPU, err) | ||
} else { | ||
rr.Limits[corev1.ResourceCPU] = q | ||
if spec.Limits.CPU != 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.
ditto
pkg/util/util.go
Outdated
glog.Errorf("failed to parse memory resource %s to quantity: %v", spec.Limits.Memory, err) | ||
} else { | ||
rr.Limits[corev1.ResourceMemory] = q | ||
if spec.Limits.Memory != 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.
ditto
@huanwei Thanks for your contribution!
The latter two are parsing storage requests for PVC, it must be non-empty because most platform needs this value to allocate a PV, so the error message is OK here. But the first one, tikv storage capacity is not required to be non-empty, if it's empty we set it to zero , but we got noisy error messages. So you need to fix this too.
|
@tennix |
/run-e2e-tests |
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
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
OK, we'll merge this PR now. @huanwei can you send another PR to fix the issue I listed above? |
@tennix Sure. I will submit a new PR to handle the storage problem. |
* remove noise error Signed-off-by: huanwei <huan@harmonycloud.cn>
* Update backup-to-s3.md * en: add IAM ENV for S3 backup and restore * update from #57 * Apply suggestions from code review Co-Authored-By: Lilian Lee <lilin@pingcap.com> Co-authored-by: Lilian Lee <lilin@pingcap.com>
Signed-off-by: huanwei huan@harmonycloud.cn
Fix issue #52 Remove noise error log message