-
Notifications
You must be signed in to change notification settings - Fork 721
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
*: update cluster_id
to a global variable
#8615
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8615 +/- ##
==========================================
+ Coverage 77.55% 77.67% +0.11%
==========================================
Files 474 475 +1
Lines 61893 61899 +6
==========================================
+ Hits 48003 48077 +74
+ Misses 10342 10281 -61
+ Partials 3548 3541 -7
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: okJiang <[email protected]>
@@ -156,25 +156,6 @@ func TestEtcdKVPutWithTTL(t *testing.T) { | |||
re.Equal(int64(0), resp.Count) | |||
} | |||
|
|||
func TestInitClusterID(t *testing.T) { |
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 we need to add a similiar test in utils/keypath
?
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 has been move to pkg/storage/endpoint/cluster_id_test.go
. The path can be discussed.
@@ -62,7 +62,6 @@ func TestSimpleJoin(t *testing.T) { | |||
members, err = etcdutil.ListEtcdMembers(ctx, client) | |||
re.NoError(err) | |||
re.Len(members.Members, 2) | |||
re.Equal(pd1.GetClusterID(), pd2.GetClusterID()) |
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 not check it?
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 is difficult to use ut to test two PD instance's clusterID. In fact, we have test it in pkg/storage/endpoint/cluster_id_test.go
, InitClusterID twice and check the two clusterID is same. TestClientClusterIDCheck
is the same reason. I plan to add a real_cluster_test to test them.
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.
To avoid to increase the size of this pr, I will file another pr to add the real cluster test about ClusterID. I have to update the test structure because we need to deploy two cluster at the same time. Current test script doesn't meet this demand.
You can continue to review this pr~
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.
Moved to pkg/utils/keypath
@@ -156,25 +156,6 @@ func TestEtcdKVPutWithTTL(t *testing.T) { | |||
re.Equal(int64(0), resp.Count) | |||
} | |||
|
|||
func TestInitClusterID(t *testing.T) { |
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 has been move to pkg/storage/endpoint/cluster_id_test.go
. The path can be discussed.
@@ -62,7 +62,6 @@ func TestSimpleJoin(t *testing.T) { | |||
members, err = etcdutil.ListEtcdMembers(ctx, client) | |||
re.NoError(err) | |||
re.Len(members.Members, 2) | |||
re.Equal(pd1.GetClusterID(), pd2.GetClusterID()) |
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 is difficult to use ut to test two PD instance's clusterID. In fact, we have test it in pkg/storage/endpoint/cluster_id_test.go
, InitClusterID twice and check the two clusterID is same. TestClientClusterIDCheck
is the same reason. I plan to add a real_cluster_test to test them.
/cc @rleungx |
If I remember correctly, we do not recommend using the global variable for cluster ID in the meeting. |
What problem does this PR solve?
Issue Number: Close #8588
What is changed and how does it work?
Check List
Tests
Release note