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

cmd: add dial timeout and verify sink connection status #561

Merged
merged 5 commits into from
May 13, 2020

Conversation

overvenus
Copy link
Member

What problem does this PR solve?

Add dial timeout and verify sink connection when creating changefeed.

Cc pingcap/ticdc#542

Check List

Tests

  • Integration test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Add client dial timeout
  • Verify sink connection when creating changefeed

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
Copy link
Member Author

/run-integration-tests

Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
Copy link
Member Author

/run-integration-tests

@@ -297,6 +297,11 @@ func newMySQLSink(ctx context.Context, sinkURI *url.URL, dsn *dmysql.Config, fil
if err != nil {
return nil, errors.Annotatef(err, "Open database connection failed, dsn: %s", dsnStr)
}
err = db.PingContext(ctx)
if err != nil {
return nil, errors.Annotatef(err, "fail to open MySQL connection")
Copy link
Contributor

Choose a reason for hiding this comment

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

also add dsnStr in the error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could leak the password.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes. Seems dsnStr in L298 also needs to be removed

Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@zier-one zier-one added the LGT1 label May 13, 2020
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
Copy link
Member Author

/run-integration-tests

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 LGT2 and removed LGT1 labels May 13, 2020
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
Copy link
Member Author

/run-integration-tests

@codecov-io
Copy link

Codecov Report

Merging #561 into master will decrease coverage by 0.0666%.
The diff coverage is 0.0000%.

@@               Coverage Diff                @@
##             master       #561        +/-   ##
================================================
- Coverage   32.2043%   32.1376%   -0.0667%     
================================================
  Files            69         69                
  Lines          6909       6914         +5     
================================================
- Hits           2225       2222         -3     
- Misses         4524       4532         +8     
  Partials        160        160                

@overvenus overvenus merged commit 29828b5 into pingcap:master May 13, 2020
@overvenus overvenus deleted the cli/timeout+verifysink branch May 13, 2020 10:44
5kbpers pushed a commit to 5kbpers/ticdc that referenced this pull request Aug 24, 2020
Signed-off-by: Neil Shen <overvenus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants