Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tests/server: reduce test time for heartbeat msg cannot be triggered #7998

Closed
wants to merge 3 commits into from
Closed

tests/server: reduce test time for heartbeat msg cannot be triggered #7998

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e98a13
fix test
HuSharp Mar 28, 2024
9635e67
Merge branch 'master' into fix_handle_msg_send
HuSharp Apr 7, 2024
18e96a6
Merge branch 'master' into fix_handle_msg_send
HuSharp Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions tests/server/cluster/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,20 +753,19 @@ func TestConcurrentHandleRegion(t *testing.T) {
re.NoError(err)
peerID, err := id.Alloc()
re.NoError(err)
regionID, err := id.Alloc()
re.NoError(err)
peer := &metapb.Peer{Id: peerID, StoreId: store.GetId()}
regionReq := &pdpb.RegionHeartbeatRequest{
Header: testutil.NewRequestHeader(clusterID),
Region: &metapb.Region{
Id: regionID,
// mock error msg to trigger stream.Recv()
Id: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it skip some processes of handle region heartbeat?

Copy link
Member Author

@HuSharp HuSharp Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but this regionReq wants to ensure the client can get the server's response.

  • only call SendScheduleCommand will send right msg by oc.hbStreams.SendMsg(region, cmd)
  • DispatchFromHeartBeat will not trigger SendScheduleCommand

Otherwise, we can only get the msg when the hbStream's keepalive is triggered, which for this test is no different than sending an error msg.

And the concurrent region heartbeat tests are handled (the purpose of this test) in the below code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember what is the purpose of this test. Does it aim to test the concurrent problem or race problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Seems like testing(including race) for concurrent running HandleRegionHeartbeat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think core logic for this test is here
https://github.com/tikv/pd/pull/7998/files#diff-4f23e8da76548a94e6779db8719386a8eabb85ad56deca5e6271a8e162652baaR789-R818

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the heartbeat process exits earlier, will it still take effect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part is just like a ping which wants to ensure the connection between the client and server.
So I think the msg of the response is unimportant :)

And I have added some breakpoints to ensure concurrent region heartbeat has been handled.

Peers: []*metapb.Peer{peer},
},
Leader: peer,
}
err = stream.Send(regionReq)
re.NoError(err)
// make sure the first store can receive one response
// make sure the first store can receive one response(error msg)
if i == 0 {
wg.Add(1)
}
Expand Down
Loading