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

Unify --filesize and --statement-size definition with mydumper's #142

Merged
merged 14 commits into from
Sep 8, 2020

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

fix #127

What is changed and how it works?

Don't only count data for statement-size and filesize options.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test

Related changes

  • Need to cherry-pick to the release branch

Release note

unify --filesize and --statement-size definition with mydumper's

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2020

CLA assistant check
All committers have signed the CLA.

v4/export/ir.go Outdated
Comment on lines 30 to 32
HasNext(currentStatementSize, currentFileSize uint64) bool
HasNextSQLRowIter(currentFileSize uint64) bool
AdjustFileRowIterSize(currentStatementSize, currentFileSize uint64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why must these passed as arguments into the iterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because currently the written size is saved in writer but the FilesizeLimit and StatementsizeLimit is saved in file row iterator

Copy link
Collaborator

Choose a reason for hiding this comment

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

gotta say this solution doesn't look good to me. the SQLRowIter should only care about pumping out *sql.Rows, it should be the CSVWriter/SQLWriter's responsibility to know when to add the statement separator and when to create a new file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I will take thought on how to refine this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised, PTAL again

Copy link
Collaborator

@kennytm kennytm 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

Comment on lines 91 to 94
if b.fileSizeLimit != UnspecifiedSize && b.currentFileSize >= b.fileSizeLimit {
return true
}
return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if b.fileSizeLimit != UnspecifiedSize && b.currentFileSize >= b.fileSizeLimit {
return true
}
return false
return b.fileSizeLimit != UnspecifiedSize && b.currentFileSize >= b.fileSizeLimit

Comment on lines 98 to 104
if b.fileSizeLimit != UnspecifiedSize && b.currentFileSize >= b.fileSizeLimit {
return true
}
if b.statementSizeLimit != UnspecifiedSize && b.currentStatementSize >= b.statementSizeLimit {
return true
}
return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@kennytm kennytm merged commit bc33935 into pingcap:master Sep 8, 2020
@lichunzhu lichunzhu deleted the unifyFileSize branch October 2, 2020 03:15
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
…ingcap#142)

* reduce bound address times to save more time

* tmp

* unify file size

* fix ut

* fix bug that escapeBackSlash not used for rows arguments

* tmp

* move filesize and statementsize argument to writepipe

* fix format

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
…ingcap#142)

* reduce bound address times to save more time

* tmp

* unify file size

* fix ut

* fix bug that escapeBackSlash not used for rows arguments

* tmp

* move filesize and statementsize argument to writepipe

* fix format

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
…ingcap#142)

* reduce bound address times to save more time

* tmp

* unify file size

* fix ut

* fix bug that escapeBackSlash not used for rows arguments

* tmp

* move filesize and statementsize argument to writepipe

* fix format

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
…ingcap#142)

* reduce bound address times to save more time

* tmp

* unify file size

* fix ut

* fix bug that escapeBackSlash not used for rows arguments

* tmp

* move filesize and statementsize argument to writepipe

* fix format

* address comment
tisonkun pushed a commit to tisonkun/tidb that referenced this pull request Oct 20, 2021
…ingcap/dumpling#142)

* reduce bound address times to save more time

* tmp

* unify file size

* fix ut

* fix bug that escapeBackSlash not used for rows arguments

* tmp

* move filesize and statementsize argument to writepipe

* fix format

* address comment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify --filesize and --statement-size definition with mydumper's
3 participants