add more logs #676
add more logs #676
Conversation
|
/run-e2e-tests |
|
/run-e2e-tests |
pkg/manager/member/tidb_failover.go
Outdated
| @@ -40,6 +41,7 @@ func (tf *tidbFailover) Failover(tc *v1alpha1.TidbCluster) error { | |||
| _, exist := tc.Status.TiDB.FailureMembers[tidbMember.Name] | |||
| if exist && tidbMember.Health { | |||
| delete(tc.Status.TiDB.FailureMembers, tidbMember.Name) | |||
| glog.Errorf("tidb failover: delete %s from tidb failoverMembers", tidbMember.Name) | |||
|
all these logs are "state changes", they can help us diagnose problems |
|
/run-e2e-tests |
pkg/manager/member/utils.go
Outdated
| @@ -182,6 +182,7 @@ func serviceEqual(new, old *corev1.Service) (bool, error) { | |||
| // setUpgradePartition set statefulSet's rolling update partition | |||
| func setUpgradePartition(set *apps.StatefulSet, upgradeOrdinal int32) { | |||
| set.Spec.UpdateStrategy.RollingUpdate = &apps.RollingUpdateStatefulSetStrategy{Partition: &upgradeOrdinal} | |||
| glog.Infof("set %s/%s partition to %d successfully", set.GetNamespace(), set.GetName(), upgradeOrdinal) | |||
There was a problem hiding this comment.
I think it's not a good idea to log in the utility function. log when the API operation has done?
There was a problem hiding this comment.
removed the successfully word. ptal
| @@ -90,8 +91,10 @@ func (opc *orphanPodsCleaner) Clean(tc *v1alpha1.TidbCluster) (map[string]string | |||
|
|
|||
| err = opc.podControl.DeletePod(tc, pod) | |||
| if err != nil { | |||
| glog.Errorf("orphan pods cleaner: failed to clean orphan pod: %s/%s, %v", ns, podName, err) | |||
There was a problem hiding this comment.
I generally prefer to annotate errors: that way there is a single error data structure with all the information that can be logged in one spot, and if there is an API request sent back in the response.
There was a problem hiding this comment.
Yes, our error logs style need a lot of improvements. We may discuss it in a new issue?
This pr tries to add detailed logs when something changes to help us diagnose problems.
There was a problem hiding this comment.
It is up to you. I think either way you should get a detailed log to help diagnose problems. If there is concurrent logging annotation will be simpler to understand.
There was a problem hiding this comment.
what is "logging annotation"?
| @@ -128,18 +129,21 @@ func (h *ha) Filter(instanceName string, pod *apiv1.Pod, nodes []apiv1.Node) ([] | |||
| // replicas less than 3 cannot achieve high availability | |||
| if replicas < 3 { | |||
| minNodeNames = append(minNodeNames, nodeName) | |||
| glog.Infof("replicas is %d, add node %s to minNodeNames", replicas, nodeName) | |||
There was a problem hiding this comment.
Loglines printed here are unnecessary, at line 109 we know the replicas of the component, if the replicas is smaller than 3, all nodes will be added into minNodeNames.
How about making it simpler, we can skip the for loop if the replicas is smaller than 3 and print one informative log?
There was a problem hiding this comment.
How to skip the for loop? we must range the nodeMap anyway
There was a problem hiding this comment.
hmm, you're right, we need to get node name
pkg/scheduler/predicates/ha.go
Outdated
| continue | ||
| } | ||
|
|
||
| podsCount := len(podNames) | ||
| if podsCount+1 >= int(replicas+1)/2 { | ||
| glog.Infof("node %s podsCount is %d, skipping", nodeName, podsCount) |
There was a problem hiding this comment.
print the int(replicas+1)/2 too?
There was a problem hiding this comment.
replicas has been printed in 109.
|
/run-e2e-tests |
1 similar comment
|
/run-e2e-tests |
(cherry picked from commit 41bceb5)
What problem does this PR solve?
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
Does this PR introduce a user-facing change?: