Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

br: add lock file #358

Merged
merged 4 commits into from
Jun 17, 2020
Merged

br: add lock file #358

merged 4 commits into from
Jun 17, 2020

Conversation

3pointer
Copy link
Collaborator

What problem does this PR solve?

Fix #328
Add a lock file at beginning, to make sure other backup task won't be able to start backup.

What is changed and how it works?

Add a lock file when storage created.

Check List

Tests

  • Integration test

Release Note

  • Fix the issue that fast fail when multiple backup task start backup on same path.

@3pointer 3pointer requested a review from kennytm June 16, 2020 03:46
@3pointer
Copy link
Collaborator Author

Maybe we should move backupmeta check, since it not necessary from now.

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   72.30%   72.30%           
=======================================
  Files          51       51           
  Lines        5687     5687           
=======================================
  Hits         4112     4112           
  Misses       1085     1085           
  Partials      490      490           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1bf32f...e1e3694. Read the comment docs.

@kennytm
Copy link
Collaborator

kennytm commented Jun 16, 2020

cc #151 the previous attempt.

@kennytm
Copy link
Collaborator

kennytm commented Jun 16, 2020

LGTM but the file name sounds odd.

Unlike a typical lock file, we do not delete backup.lock after backup has completed.

@kennytm kennytm added the status/LGT1 LGTM1 label Jun 16, 2020
@3pointer
Copy link
Collaborator Author

LGTM but the file name sounds odd.

Unlike a typical lock file, we do not delete backup.lock after backup has completed.

yes, if we want to delete file we need add delete method for external storage, I wonder is it necessary, because it does make sense even one backup finished, other backup tasks still won't be able to use same path. maybe change lock file to another name is better, do you have any advise?

Copy link
Member

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

@@ -122,6 +122,11 @@ func (bc *Client) GetTS(ctx context.Context, duration time.Duration, ts uint64)
return backupTS, nil
}

// SetLockFile set write lock file.
func (bc *Client) SetLockFile(ctx context.Context) error {
return bc.storage.Write(ctx, utils.LockFile, []byte{})
Copy link
Member

Choose a reason for hiding this comment

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

Could you write some warnings to the file? Eg. DO NOT DELETE and explain what the file does.

@kennytm kennytm requested a review from overvenus June 17, 2020 11:29
@overvenus overvenus merged commit 56e151a into pingcap:master Jun 17, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 failed

@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #366

ti-srebot added a commit that referenced this pull request Jun 18, 2020
* br: add lock file

* fix ci

* address comment

Co-authored-by: luancheng <luancheng@pingcap.com>
ti-srebot added a commit that referenced this pull request Jul 8, 2020
* tests: add retry on start services (#367)

* tests: add retry on start services (#367)

* tests: add retry on start services

* tests: forgot to add retry count :/

* tests: try to fix br_other

* tests: use return 1 instead of exit 1

* tests: replace tidb_status_addr to another port

* tests: meltdown tiflash tests

(waiting for tiflash folks help)

* tests: reset tiflash config to tiup default

* tests: rename tiflash log file

* tests: change tiflash status port

* tests: seek tiflash if possiable

* tests: fix tiflash

* tests: fix a typo

* tests: shrink capacity

* tests: rollback tiflash config

* tests: rollback...

* tests: let all go back to start

Can we find what make things bad...?

* tests: rollback...

* tests: disable tiflash test for some time

* tests: rename a variable

I *love* bash...

* tests: use tiflash http port to test whether tiflash started

Co-authored-by: 3pointer <luancheng@pingcap.com>
Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BR check backup directory is empty before backup start
4 participants