Allow more repository backends #4312
Conversation
|
Hi @mikix! Thanks for working on this! A few comments:
|
mikix
force-pushed
the
backends
branch
2 times, most recently
from
1bf3109
to
4475092
Compare
|
Alright. This PR is at a good checkpoint. It is stage 1 of 3 of my planned work. This simply adds a new Repository abstraction in front of RemoteRepository and LocalRepository (nee Repository). And then all the code changes to support the main code not having to know particulars of a given repository backend. I tried to be conservative where I could. All tests pass. Though I didn't do extensive manual testing beyond some smoke-testing. This PR doesn't add any new features and is a reasonable change in its own right. That said, you mentioned coming up on 1.2, and I'd vote in favor of waiting until after 1.2 to land this. Non-trivial refactors by people new to the codebase always make me nervous. I'm fine with rebasing in the meantime. Regardless, if you feel like reading some code, this can be reviewed ahead of time. Stage two will be another refactor PR to add a DirectAccessRepository abstract class as a superclass of LocalRepository. This new superclass will hopefully hold all the general repository logic and leave LocalRepository as just a get/put/delete layer for POSIX files. And stage three will finally be a feature PR adding a new GIO backend that will also inherit DirectAccessRepository. I'll begin plotting stage two. |
mikix
changed the title
Codecov Report
@@ Coverage Diff @@
## master #4312 +/- ##
==========================================
+ Coverage 83.87% 83.95% +0.07%
==========================================
Files 37 38 +1
Lines 9764 9863 +99
Branches 1621 1626 +5
==========================================
+ Hits 8190 8280 +90
- Misses 1102 1111 +9
Partials 472 472
Continue to review full report at Codecov.
|
|
Please rebase on current master to get the tests in a better state (see my recent PRs) and to resolve conflicts. |
mikix
force-pushed
the
backends
branch
2 times, most recently
from
a1f2e8a
to
9d42f68
Compare
|
Rebased! |
mikix
force-pushed
the
backends
branch
2 times, most recently
from
eec327d
to
672166f
Compare
This is in preparation for more work to refactor the repository backends. But done as a separate commit so git can track them easier.
- Move repository.py -> repositories/local.py (LocalRepository) - Move remote.py -> repositories/remote.py - Add repository.py with Repository abstraction that proxies access to the above two classes - Have RPCErrors act like normal Errors so that general code doesn't have to know its particulars - Likewise, handle servers too old for nonce support internally to RemoteRepository - Have the Repository object say whether its backend is remote or not rather than hardcoding knowledge from class names
|
I had started something like this also long ago, when I thought I could add rclone-support directly into borg (Spoiler: I couldn't. Way too bad at python, I just silently gave up). But the idea to take the first step to have different repository implementations is really cool. Assuming the PR is split up into two or three more commits and rebased on the current code, would you consider it something worth merging @ThomasWaldmann ? |
|
Likely will be superseded by #8332. As we can see there and in quite some previous borg2 changes, a lot of changes were needed to get to that repository flexibility as seen there. |
|
#8332 was merged into master (borg2), closing this. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4312 +/- ##
==========================================
+ Coverage 83.87% 83.95% +0.07%
==========================================
Files 37 38 +1
Lines 9764 9863 +99
Branches 1621 1626 +5
==========================================
+ Hits 8190 8280 +90
- Misses 1102 1111 +9
Partials 472 472 ☔ View full report in Codecov by Sentry. |
I'm filing this PR now before it's ready to get course-corrected if I'm way off base. Refactor PRs like this one are highly liable to be considered off-base. :)
I'm preparing a refactor PR (this one) to abstract the concept of a Repository in the code. It renames and moves the existing two repository backends:
RemoteRepository in remote.py -> RemoteRepository in respositories/remote.py
Repository in repository.py -> LocalRepository in repositories/local.py
And then adds a new Repository class in repository.py as an interface between the rest of the system and the backend repository class. Ideally as little of the code outside repositories/ needs to know anything about what's inside it, and they all just deal with the main Repository class.
I'll also probably add a FileRepository class that LocalRepository will subclass. This will let new protocol backends just implement the file access primitives needed (get/put/delete).
Before marking this as ready for review, I'll also prepare a feature PR that adds a new backend (likely for gio, which will give access to all sorts of protocols in one swoop). I want to prepare both before either lands in order to test the abstraction and let me see if borg's performance completely tanks with direct remote access.
Would close issue #1070.