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

Refactor database code to support standalone batches, transactions. #861

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

smira
Copy link
Contributor

@smira smira commented Aug 1, 2019

This is spin-off of changes from #459.

Transactions are not being used yet, but batches are updated to work
with the new API.

database/ package was refactored to split abstract interfaces and
implementation via goleveldb. This should make it easier to implement
new database types.

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #861 into master will increase coverage by <.01%.
The diff coverage is 77.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
+ Coverage   64.06%   64.07%   +<.01%     
==========================================
  Files          51       54       +3     
  Lines        6565     6588      +23     
==========================================
+ Hits         4206     4221      +15     
- Misses       1854     1860       +6     
- Partials      505      507       +2
Impacted Files Coverage Δ
deb/contents.go 0% <0%> (ø) ⬆️
context/context.go 11.28% <0%> (ø) ⬆️
deb/package_collection.go 51.51% <100%> (ø) ⬆️
database/goleveldb/batch.go 100% <100%> (ø)
deb/publish.go 63.39% <66.66%> (ø) ⬆️
database/goleveldb/transaction.go 72.72% <72.72%> (ø)
database/goleveldb/database.go 76.92% <76.92%> (ø)
database/goleveldb/storage.go 78.82% <78.82%> (ø)
... and 2 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 26098f6...c5bb143. Read the comment docs.

Copy link
Contributor

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

Could you move the changing of mirror into another PR? Otherwise this makes reviewing harder as there are code and test changes which are not relevant to each other.

In terms of database interface - do we want to support another database anytime soon?
In my opinion in golang interface should only be defined if there are actually used as that's the power of golang that those can be define at any time fairly easily. So if we don't implement another database any time soon I would not define interfaces.

These are just some initial comments - I will have to look into the changes when i have some more time at hand.

@smira
Copy link
Contributor Author

smira commented Aug 5, 2019

Could you move the changing of mirror into another PR? Otherwise this makes reviewing harder as there are code and test changes which are not relevant to each other.

Yep, makes sense. I will split. I was trying to fix the build in Travis CI, but looks like the failures are still there. Going to add retries to aptly on network errors.

In terms of database interface - do we want to support another database anytime soon?

That was the initial plan.

In my opinion in golang interface should only be defined if there are actually used as that's the power of golang that those can be define at any time fairly easily. So if we don't implement another database any time soon I would not define interfaces.

Interface was there since the inception of aptly, I just moved things around so that's it more obvious what is interface, and what is the implementation.

I had plans to add other types of databases, in fact it might make sense to support something distributed (Redis?), but this is not my goal for this PR of course.

Interface plays another important role here: it abstracts away goleveldb, so we don't end up using features in a random way, it's clear what's being used and what not.

@sliverc
Copy link
Contributor

sliverc commented Aug 5, 2019

@smira My quick look over the changes was a bit too quick... 😉 let's leave the interfaces as they were before then.

@smira
Copy link
Contributor Author

smira commented Aug 5, 2019

Split not related commits into #863, #864

This is spin-off of changes from aptly-dev#459.

Transactions are not being used yet, but batches are updated to work
with the new API.

`database/` package was refactored to split abstract interfaces and
implementation via goleveldb. This should make it easier to implement
new database types.
@smira
Copy link
Contributor Author

smira commented Aug 7, 2019

@sliverc updated, should be ready for the review now

Copy link
Contributor

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

👍

@smira smira merged commit 67e3895 into aptly-dev:master Aug 8, 2019
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.

2 participants