Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

streamer: add heartbeat for local streamer #1404

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Jan 27, 2021

What problem does this PR solve?

currently relay will ignore heartbeat event before writing to relay logs. so when relay enabled, there's no heartbeat event which causes syncer can't advance its checkpoint when upstream is idle.

What is changed and how it works?

peroidically send heartbeat event

Check List

Tests

  • Unit test
  • Manually test (only do an insert in upstream, and check checkpoint advanced after 30s)

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@lance6716 lance6716 added needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 27, 2021
@lance6716
Copy link
Collaborator Author

PTAL @3pointer @lichunzhu

select {
case <-heartbeatTicker.C:
Copy link
Contributor

Choose a reason for hiding this comment

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

<- time.After(heartbeatInterval)

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

/lgtm

@lichunzhu lichunzhu added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 28, 2021
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jan 28, 2021
@lance6716
Copy link
Collaborator Author

/merge

@ti-srebot
Copy link

@lance6716 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 0

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lance6716 lance6716 merged commit 7c7097c into pingcap:master Jan 28, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Jan 28, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1406

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Jan 28, 2021
lance6716 pushed a commit that referenced this pull request Jan 28, 2021
@lance6716 lance6716 added this to the v2.0.2 milestone Jan 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants