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

resume interrupted backups #310

Merged
merged 12 commits into from Oct 14, 2015
Merged

resume interrupted backups #310

merged 12 commits into from Oct 14, 2015

Conversation

fd0
Copy link
Member

@fd0 fd0 commented Sep 27, 2015

This breaks out some commits from #252 (which was clearly too large to be done in one PR). The code in this PR adds resuming of interrupted backups by regularly writing an index (utilizing the new supersedes field) and reading these when a backup should be resumed.

Closes #59

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

@episource I've handled the bug you discovered, thanks again.

@episource
Copy link
Contributor

Is a RWMutex reantrant? The documentation does not say anything about this...

~~Context: ~~
AddInFlight acquires a RW-Lock (mi.inFlight.Lock()) and then calls Has, which tries to acquire a Read-Lock (mi.idxMutex.RLock()). Without RWMutex being reantrant this should fail/dead-lock.

Assuming go locks to be reantrant I tried the same while implementing my draft of StoreInProgress. Due to that I learned the hard way, that my assumption was wrong and Mutex (not RWMutex) is not reantrant.

Edit: Reading my comment again, I noticed that two different lock objects are involved.

Edit: Nothing is locked twice. Everything is alright.

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

Uhoh. I'll have a look, thanks again!

@episource
Copy link
Contributor

No, just forget my comment. It's plain wrong.

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

Hm, I'd expect that acquiring the r lock while an r/w lock is already held leads to a deadlock...

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

Ah, indeed, now I see it.

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

Any other comments? Otherwise I'll merge this branch now :)

@episource
Copy link
Contributor

Not from my side...

@fd0
Copy link
Member Author

fd0 commented Oct 14, 2015

cool

@fd0 fd0 merged commit 1dd731f into master Oct 14, 2015
fd0 added a commit that referenced this pull request Oct 14, 2015
@fd0 fd0 deleted the resume-backups branch October 14, 2015 19:53
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.

None yet

2 participants