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

drainer: Fix #724, Enable drainer to purge old incremental backup data on disk #885

Merged
merged 6 commits into from
Feb 7, 2020

Conversation

suzaku
Copy link
Contributor

@suzaku suzaku commented Jan 15, 2020

What problem does this PR solve?

Enable drainer to purge old incremental backup data on disk.

What is changed and how it works?

  1. Add a retention-time config to specify how long binlog files should be kept in drainer. Backward-compatibility is maintained by treating non-positive values of retention-time as no GC.
  2. Currently retention-time is of type int, which is consistent with the pump GC configuration. We may later introduce p8s style time format for both GC and retention-time and use days as the default unit to maintain backward-compatibility.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

    The GC method is changed to two new methods GCByTIme and GCByPos.

Related changes

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

@suzaku
Copy link
Contributor Author

suzaku commented Jan 15, 2020

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Jan 16, 2020

@shinnosuke-okada @july2993 PTAL

cmd/drainer/drainer.toml Outdated Show resolved Hide resolved
drainer/sync/pb.go Outdated Show resolved Hide resolved
@suzaku
Copy link
Contributor Author

suzaku commented Feb 3, 2020

/run-all-tests

@suzaku
Copy link
Contributor Author

suzaku commented Feb 3, 2020

/run-integration-tests

@suzaku
Copy link
Contributor Author

suzaku commented Feb 3, 2020

@july2993 PTAL

@IANTHEREAL
Copy link
Collaborator

Rest LGTM

suzaku and others added 2 commits February 4, 2020 10:49
Co-Authored-By: Ian <ArGregoryIan@gmail.com>
Co-Authored-By: Ian <ArGregoryIan@gmail.com>
}

// NewPBSyncer sync binlog to files
func NewPBSyncer(dir string, tableInfoGetter translator.TableInfoGetter) (*pbSyncer, error) {
func NewPBSyncer(dir string, retentionDays int, tableInfoGetter translator.TableInfoGetter) (*pbSyncer, error) {
Copy link

Choose a reason for hiding this comment

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

exported func NewPBSyncer returns unexported type *sync.pbSyncer, which can be annoying to use

}

// NewPBSyncer sync binlog to files
func NewPBSyncer(dir string, tableInfoGetter translator.TableInfoGetter) (*pbSyncer, error) {
func NewPBSyncer(dir string, retentionDays int, tableInfoGetter translator.TableInfoGetter) (*pbSyncer, error) {
Copy link

Choose a reason for hiding this comment

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

exported func NewPBSyncer returns unexported type *sync.pbSyncer, which can be annoying to use

@suzaku
Copy link
Contributor Author

suzaku commented Feb 4, 2020

/run-all-tests

Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@sokada1221 sokada1221 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :)

Sorry for the late response - was getting bombarded with PR reviews these days...

@suzaku suzaku merged commit ff59e72 into pingcap:master Feb 7, 2020
@sre-bot
Copy link

sre-bot commented Feb 7, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link

sre-bot commented Feb 7, 2020

cherry pick to release-3.1 failed

@suzaku suzaku deleted the periodic-gc-for-file-sync branch February 7, 2020 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants