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

Introduce Database interface. #267

Merged
merged 3 commits into from
Oct 23, 2020
Merged

Conversation

jholdstock
Copy link
Member

All required database functionality is now defined in a new Database interface. Almost all of the production code has been updated to only handle this interface, rather than any concrete db implementations. This should enable the bbolt implementation to be switched out for a postgres implementation in future.

A big step for #257

All required database functionality is now defined in a new Database interface. Almost all of the production code has been updated to only handle this interface, rather than any concrete db implementations. This should enable the bbolt implementation to be switched out for a postgres implementation in future.
pool/database.go Outdated
loadLastPaymentCreatedOn() (int64, error)

// Account
FetchAccount(id string) (*Account, error)
Copy link
Member

Choose a reason for hiding this comment

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

I suspect almost all the functions required by the interface here could be unexported. Will look into doing that if you decide against it in this PR.

pool/hub.go Outdated
@@ -142,7 +141,7 @@ type HubConfig struct {
type Hub struct {
clients int32 // update atomically.

db *bolt.DB
db Database
Copy link
Member

@dnldd dnldd Oct 22, 2020

Choose a reason for hiding this comment

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

Just noticed he hub config and the hub all have references to the db, likely oversight on my part. I think we should remove one however: the hub reference I'm thinking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I think removing the one from config makes more sense. The database is one dependency, the config is another. Each is a saparate thing with distinct responsibilities - the config holds simple values which define how the hub should behave, the database is a persistent storage device.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I wont make that change because that will conflict with how the rest of the dcrpool components are setup. They all have the database stored in their config, so I will maintain that for hub as well.

pool/job.go Outdated
@@ -48,10 +48,10 @@ func NewJob(header string, height uint32) *Job {
}

// FetchJob fetches the job referenced by the provided id.
func FetchJob(db *bolt.DB, id string) (*Job, error) {
func (db *BoltDB) FetchJob(id string) (*Job, error) {
Copy link
Member

@dnldd dnldd Oct 22, 2020

Choose a reason for hiding this comment

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

Perhaps all the BoltDB interface implementations need to go into a separate file? Or do you plan on adding the postgres implementations to the struct files as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's something we need to discuss. We could take either approach, but tbh I think both are kind of ugly 😂

I havent moved the funcs in this PR simply because it would make the changeset massive, and the review painful.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing, let's resolve this in a separate PR.

@jholdstock jholdstock merged commit 74774c2 into decred:master Oct 23, 2020
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