-
Notifications
You must be signed in to change notification settings - Fork 720
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
test: reduce redundant code about etcd server and logger #7007
Conversation
Signed-off-by: lhy1024 <[email protected]>
Signed-off-by: lhy1024 <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: lhy1024 <[email protected]>
Signed-off-by: lhy1024 <[email protected]>
pkg/utils/etcdutil/testutil.go
Outdated
// Start the new etcd member. | ||
etcd2, err := embed.StartEtcd(cfg2) | ||
if err != nil { | ||
re.Contains(err.Error(), "error validating peerURLs") |
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.
When will this error happen?
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.
In #6890, it occurs with "member count is unequal"
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.
Is the error expected?
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.
No, we have sent MemberAdd
before starting a new etcd server, but it still returns error validating peerURLs
with member count is unequal
in #6890 case, which frequency is extremely low, only appearing once.
Signed-off-by: lhy1024 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #7007 +/- ##
==========================================
- Coverage 74.46% 74.37% -0.10%
==========================================
Files 435 435
Lines 46218 46264 +46
==========================================
- Hits 34418 34408 -10
- Misses 8796 8838 +42
- Partials 3004 3018 +14
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: lhy1024 <[email protected]>
} | ||
|
||
// MustAddEtcdMember is used to add a new etcd member to the cluster. | ||
func MustAddEtcdMember(t *testing.T, cfg1 *embed.Config, client *clientv3.Client) *embed.Etcd { |
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 call it Mustxxx?
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.
Do you have any good ideas about the name of this test function?
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.
Here is no guarantee that the member is added.
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 will retry three times, and if it fails all three times, it will report an error re.NoError(err, "addEtcdMemberWithRetry failed after retry")
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 use another way to make the test stable, which is to wait member number to be normal before adding a new node.
Signed-off-by: lhy1024 <[email protected]>
Signed-off-by: lhy1024 <[email protected]>
/merge |
@lhy1024: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: 3923233
|
close tikv#6890 Signed-off-by: lhy1024 <[email protected]>
What problem does this PR solve?
Issue Number: Close #6890 #6936
What is changed and how does it work?
Check List
Tests
Release note