◐ Shell
reader mode source ↗
Skip to content

bpo-27645: Supporting native backup facility of SQLite#377

Closed
lelit wants to merge 21 commits into
python:masterfrom
lelit:sqlite-backup-api-v2
Closed

bpo-27645: Supporting native backup facility of SQLite#377
lelit wants to merge 21 commits into
python:masterfrom
lelit:sqlite-backup-api-v2

Conversation

@lelit

@lelit lelit commented Mar 1, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

This is a set of patches (rebased on current master) that adds a new backup() method on sqlite3.Connection.

https://bugs.python.org/issue27645

@the-knights-who-say-ni

the-knights-who-say-ni commented Mar 1, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
  4. If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

@mention-bot

mention-bot commented Mar 1, 2017

Copy link
Copy Markdown

@lelit, thanks for your PR! By analyzing the history of the files in this pull request, we identified @berkerpeksag, @zware, @gvanrossum, @avassalotti and @orsenthil to be potential reviewers.

@lelit

lelit commented Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

I configured my GitHub username on b.p.o.

@lelit

lelit commented Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

Thank you @berkerpeksag!

lelit added 4 commits March 3, 2017 10:09
@lelit
Parametrize the name of the database involved in the backup …
e748942 
Suggested by Aviv Palivoda.
@lelit
Test propagation of exception raised in backup's progress callback
426dad4
@lelit
Use a better name for variable
895726b
@lelit
Do not delay next iteration if the result was OK …
77dfa39 
When sqlite3_backup_step() returns OK it means that it was able to do it's work but there are
further pages to be copied: in such case it's better to immediately start the next iteration,
without opening a window that allows other threads to go on and possibly locking the database.

Suggested by Aviv Palivoda.
@lelit
Pass also the current status of the ongoing backup to the progress ca… …
ff1609b 
…llback

Add new SQLITE_DONE integer constant to the module, that could be used by the callback to
determine whether the backup is terminated.
33 hidden items Load more…
lelit added 2 commits March 4, 2017 10:45
@lelit
Slightly different way handling backup step's error state …
d688f20 
This should address Aviv's concerns about how the sqlite3_backup_step() errors are handled.
@lelit
When an error occurs while the backup is going on, remove the target … …
4ed6c31 
…file
@lelit

lelit commented Mar 24, 2017

Copy link
Copy Markdown
Contributor Author

Is there anything else I could/should do to see this accepted?

@lelit

lelit commented Oct 5, 2017

Copy link
Copy Markdown
Contributor Author

FYI, I rebased these commits on top of current master: https://github.com/lelit/cpython/tree/sqlite-backup-api-v3

@lelit

lelit commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@Mariatta I'm confused by that "added needs rebase and removed needs rebase, what should I do?
Also, given that current implementation (rebased on master of three weeks ago) lives in a different branch (https://github.com/lelit/cpython/compare/sqlite-backup-api-v3) should I do something wrt this PR?

@Mariatta

Mariatta commented Oct 30, 2017

Copy link
Copy Markdown
Member

@lelit We can only review PRs in CPython repo. Currently this PR has conflict. If the conflict is resolved in a different branch, maybe you can create the PR for the other branch?
Although I would prefer that the conflict is resolved in the same branch so we get to keep the past discussions in the same PR.

@lelit

lelit commented Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

Does that mean you'd prefer me to rebase the branch I opened the PR with?

@brettcannon brettcannon closed this Nov 2, 2017
@brettcannon brettcannon reopened this Nov 2, 2017
@brettcannon

brettcannon commented Nov 2, 2017

Copy link
Copy Markdown
Member

@lelit There's a conflict because you edited Misc/NEWS and it will no longer merge cleanly as-is. See https://devguide.python.org/committing/?highlight=news.d#what-s-new-and-news-entries on how to fix this.

@brettcannon brettcannon changed the title bpo-27645: Supporting native backup facility of SQLite Nov 2, 2017
@brettcannon brettcannon changed the title bpo-27645: Supporting native backup facility of SQLite. Nov 2, 2017
@lelit

lelit commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

@brettcannon, I rebased the branch into https://github.com/lelit/cpython/compare/sqlite-backup-api-v3
rewriting the NEWS change using the new tool.

As said in this comment I'm sorry about the fuss, but it's not clear to me whether I should have closed this PR and reopened a new one on the v3 branch, or if instead I should have rewritten the same v2 branch.

As always, I'm willing to do whatever is easier/more correct for you!

@brettcannon

brettcannon commented Nov 2, 2017

Copy link
Copy Markdown
Member

@lelit I'm not sure what 'v2' and 'v3' are referring to (some personal branches in your fork you have?), but if you look at https://github.com/python/cpython/pull/377/files you will see that the branch your PR is based on has not been updated in regards to the NEWS file.

If it's easier, you can just close this PR, open a new one if you have some other branch that's in better shape, and then refer to this old PR in the new PR's opening comment.

@lelit

lelit commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

Ok, doing that immediately.

@lelit lelit mentioned this pull request Nov 2, 2017
Merged
@lelit

lelit commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

As suggested by Brett, I'm closing this PR, replaced by #4238.

@lelit lelit closed this Nov 2, 2017
@python python deleted a comment from richmoore1962 Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag requested changes

+2 more reviewers

@dgilman dgilman dgilman left review comments

@palaviv palaviv palaviv requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@lelit @the-knights-who-say-ni @mention-bot @Mariatta @brettcannon @berkerpeksag @dgilman @palaviv