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

support --max-idle-time argument #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

support --max-idle-time argument #326

wants to merge 1 commit into from

Conversation

dbakit
Copy link

@dbakit dbakit commented Aug 3, 2021 •
edited
Loading

See #282

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@CLAassistant
Copy link

CLAassistant commented Aug 3, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot
Copy link
Member

Welcome @dba-kit!

It looks like this is your first PR to pingcap/dumpling 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/dumpling. 😃

@lichunzhu
Copy link
Contributor

@dba-kit Could you please help sign the CLA?

@lance6716
Copy link
Collaborator

this commit de1f43d is associated with another email rather than your GitHub account. You can change commit email address and fix this problem (maybe file another PR).

@dbakit
Copy link
Author

dbakit commented Aug 4, 2021

@dba-kit Could you please help sign the CLA?

done, but I find I delete a line in v4/export/config.go by mistake

_ = flags.MarkHidden(flagReadTimeout)

lichunzhu
lichunzhu reviewed Aug 4, 2021
View reviewed changes
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.

Rest LGTM

v4/export/config.go
@@ -242,7 +244,7 @@ func (conf *Config) DefineFlags(flags *pflag.FlagSet) {
flags.StringToString(flagParams, nil, `Extra session variables used while dumping, accepted format: --params "character_set_client=latin1,character_set_connection=latin1"`)
flags.Bool(FlagHelp, false, "Print help message and quit")
flags.Duration(flagReadTimeout, 15*time.Minute, "I/O read timeout for db connection.")
_ = flags.MarkHidden(flagReadTimeout)
flags.Duration(flagMaxIdleTime, 600*time.Second, "max idle time for db connection.")
Copy link
Contributor

Choose a reason for hiding this comment

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

We'd better keep flagReadTimeout hidden. What's more, we'd better keep flagMaxIdleTime hidden, too. We can "un-hidden" these arguments if they are frequently used.

Copy link
Author

Choose a reason for hiding this comment

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

err... so those arguments still unseen to others, unless they look the source code?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add these flags to official documents.

@lichunzhu lichunzhu added the needs-cherry-pick-release-5.2 Type: Need cherry pick to release-5.2 label Oct 12, 2021
@lichunzhu
Copy link
Contributor

@dba-kit hello, Dumpling has been moved to https://github.com/pingcap/tidb/tree/master/dumpling now. Could you please help move this PR to pingcap/tidb?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lichunzhu lichunzhu lichunzhu left review comments

@kennytm kennytm Awaiting requested review from kennytm

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 Type: Need cherry pick to release-5.2 size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dbakit @ti-chi-bot @CLAassistant @lichunzhu @lance6716