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.

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

cdc, pkg/txnutil: skip resolve lock during puller initialization #910

Merged
merged 10 commits into from
Sep 4, 2020

Conversation

overvenus
Copy link
Member

What problem does this PR solve?

This PR skips resolve lock during puller initialization and suppress some verbose log.

What is changed and how it works?

By adding a new API to check whether a puller is initialized or not.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has interface methods change

Related changes

  • Need to cherry-pick to the release branch

Release note

  • No release note.

Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus added the status/ptal Could you please take a look? label Sep 1, 2020
@overvenus
Copy link
Member Author

/run-integration-tests

Comment on lines 233 to 236
log.Info("puller is initialized",
zap.Duration("duration", time.Since(start)),
zap.String("changefeedid", changefeedID),
zap.Uint64("resolvedTs", resolvedTs))
Copy link
Contributor

Choose a reason for hiding this comment

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

Record tableID is better

@overvenus
Copy link
Member Author

PTAL

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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2020
Signed-off-by: Neil Shen <overvenus@gmail.com>
@liuzix
Copy link
Contributor

liuzix commented Sep 4, 2020

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 4, 2020
@amyangfei
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 4, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 914

@ti-srebot
Copy link
Contributor

/run-all-tests

cdc/kv/client.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #910 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #910   +/-   ##
===========================================
  Coverage   33.8676%   33.8676%           
===========================================
  Files            99         99           
  Lines         12165      12165           
===========================================
  Hits           4120       4120           
  Misses         7641       7641           
  Partials        404        404           

@overvenus
Copy link
Member Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 914
  • 909

@ti-srebot
Copy link
Contributor

/run-all-tests

@overvenus
Copy link
Member Author

/run-integration-tests

@ti-srebot
Copy link
Contributor

@overvenus merge failed.

@overvenus overvenus merged commit 0608f39 into pingcap:master Sep 4, 2020
@overvenus overvenus deleted the reduce-resolve-lock-and-log branch September 4, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants