-
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
heartbeat: rebind stream. #918
Conversation
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
server/grpc_service.go
Outdated
@@ -311,9 +313,10 @@ func (s *Server) RegionHeartbeat(stream pdpb.PD_RegionHeartbeatServer) error { | |||
|
|||
hbStreams := cluster.coordinator.hbStreams | |||
|
|||
if isNew { | |||
if time.Since(lastBind) > heartbeatStreamRebindInterval { | |||
regionHeartbeatCounter.WithLabelValues(storeLabel, "report", "bind").Inc() | |||
hbStreams.bindStream(storeID, server) |
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.
any side effect if you bindStream again after the interval?
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.
Just re-assign a map entry, should be trivial.
server/grpc_service.go
Outdated
@@ -239,6 +239,8 @@ func (s *Server) StoreHeartbeat(ctx context.Context, request *pdpb.StoreHeartbea | |||
|
|||
const regionHeartbeatSendTimeout = 5 * time.Second | |||
|
|||
var heartbeatStreamRebindInterval = time.Minute |
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.
can we use a config here to speed up test?
I see that you should wait at least 1m in test to rebind.
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've set it to 3 seconds in test.
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.
oh, this is a var I just notice. Seem it is a not good way here.
PTAL @siddontang |
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
But CI failed @disksing |
I'm going to just restart it. |
Circumvent potential problems that may be occurred by reconnecting.