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

Allow more repository backends #4312

Closed
wants to merge 2 commits into from
Closed

Conversation

mikix
Copy link

@mikix mikix commented Feb 3, 2019

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.

@ThomasWaldmann
Copy link
Member

Hi @mikix!

Thanks for working on this!

A few comments:

  • when moving stuff around, I guess you'll get quite some conflicts with other PRs.
  • what's currently in master is working towards the "hydrogen" milestone and should become borg 1.2 in the not too distant future. having more backends was not planned for that milestone.
  • try to make smaller commits and have the idea of the commit in the commit comment, like "move constants to constants module"
  • a simple, 2nd local backend seems a good idea, just as PoC for the abstraction. Not sure if it is useful, but I once wrote "zborg" as a super simple implementation of the basic idea of borg and to play around with zeromq. It's backend was also very simple, but did not support all the stuff borg would have needed. https://github.com/ThomasWaldmann/zborg/blob/master/src/zborg/__main__.py
  • we also could use backend flexibility for working on the "breaking" labelled issues.

@mikix mikix force-pushed the backends branch 2 times, most recently from 1bf3109 to 4475092 Compare February 5, 2019 02:47
@mikix
Copy link
Author

mikix commented Feb 5, 2019

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 mikix changed the title WIP: allow more repository backends Allow more repository backends Feb 5, 2019
@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #4312 into master will increase coverage by 0.07%.
The diff coverage is 85.1%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/borg/cache.py 86.36% <100%> (-0.02%) ⬇️
src/borg/helpers/parseformat.py 89.59% <100%> (+0.75%) ⬆️
src/borg/archive.py 83.11% <100%> (-0.02%) ⬇️
src/borg/crypto/nonces.py 95.55% <100%> (+1.32%) ⬆️
src/borg/helpers/errors.py 100% <100%> (ø) ⬆️
src/borg/upgrader.py 62.28% <100%> (+0.21%) ⬆️
src/borg/repositories/remote.py 74.95% <65.21%> (ø)
src/borg/crypto/key.py 86.52% <66.66%> (ø) ⬆️
src/borg/archiver.py 80.99% <71.42%> (+0.02%) ⬆️
src/borg/repositories/local.py 84.26% <84.26%> (ø)
... and 5 more

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 29aec48...578a654. Read the comment docs.

@ThomasWaldmann
Copy link
Member

Please rebase on current master to get the tests in a better state (see my recent PRs) and to resolve conflicts.

@mikix mikix force-pushed the backends branch 2 times, most recently from a1f2e8a to 9d42f68 Compare February 14, 2019 02:56
@mikix
Copy link
Author

mikix commented Feb 14, 2019

Rebased!

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
@dragetd
Copy link
Contributor

dragetd commented Feb 22, 2023

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 ?

@ThomasWaldmann
Copy link
Member

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.

@ThomasWaldmann
Copy link
Member

#8332 was merged into master (borg2), closing this.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 85.10638% with 196 lines in your changes missing coverage. Please review.

Project coverage is 83.95%. Comparing base (29aec48) to head (578a654).
Report is 2953 commits behind head on master.

Files with missing lines Patch % Lines
src/borg/repositories/local.py 84.26% 108 Missing and 55 partials ⚠️
src/borg/repositories/remote.py 65.21% 15 Missing and 1 partial ⚠️
src/borg/repository.py 93.68% 7 Missing and 6 partials ⚠️
src/borg/archiver.py 71.42% 1 Missing and 1 partial ⚠️
src/borg/crypto/key.py 66.66% 2 Missing ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

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.

5 participants