Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

DB Store Abstractions #993

Closed
wants to merge 17 commits into from
Closed

DB Store Abstractions #993

wants to merge 17 commits into from

Conversation

janos
Copy link
Member

@janos janos commented Nov 12, 2018

This PR introduces a small package with database abstractions that may be used for swarm storage layer. Closes: #989.

This PR is not required to be merged into the master branch until there is an implementation that depends on it.

This PR is open for reviews and suggestions on design and new features. It is founded on requirements described in https://hackmd.io/ffBjSu8RTyKikvRO7bYrzA#.

Future generalization: #1001.

@janos janos changed the title [WIP] db store abstractions DB Store Abstractions Nov 16, 2018
swarm/shed/db.go Outdated Show resolved Hide resolved
swarm/shed/index_mock.go Outdated Show resolved Hide resolved
// But it is also returned with additional data from get function call
// and as the argument in iterator function definition.
type IndexItem struct {
Address []byte
Copy link
Member

Choose a reason for hiding this comment

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

need to add a bool field to control if Index encode/decode should actually fetch data from mock globalstore

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Great work on this @janos

There's a few comments in the body of the PR. one thing that I particularly don't really understand is that UseMockStore bool pointer. does this somehow relate to the current (on master) db layout with the mockStore component injection (which is in fact just a store). In general I would like to avoid using field names that include mock(s) in production code.

thanks <3 🙏

swarm/shed/db.go Outdated Show resolved Hide resolved
swarm/shed/db.go Outdated Show resolved Hide resolved
swarm/shed/db.go Outdated Show resolved Hide resolved
swarm/shed/db.go Outdated Show resolved Hide resolved
swarm/shed/db.go Show resolved Hide resolved
swarm/shed/index.go Outdated Show resolved Hide resolved
swarm/shed/index.go Show resolved Hide resolved
swarm/shed/index.go Outdated Show resolved Hide resolved
swarm/shed/schema.go Show resolved Hide resolved
swarm/shed/schema.go Outdated Show resolved Hide resolved
swarm/shed/index.go Show resolved Hide resolved
@acud
Copy link
Member

acud commented Nov 23, 2018

  1. @zelig why?
    1.1. Address and Data are nullable
  2. I still fail to understand what the field name signifies

@janos
Copy link
Member Author

janos commented Nov 23, 2018

I still fail to understand what the field name signifies

@justelad This field was left by my mistake after an attempt to have a mock store integration in shed. We decided that it may be better to leave this integration for localstore or a higher level package.

I will remove this field.

@janos
Copy link
Member Author

janos commented Nov 23, 2018

  1. @zelig why?
    1.1. Address and Data are nullable
  2. I still fail to understand what the field name signifies

@justelad I missed this comment while writing responses here is a brief explanation #993 (comment).

@janos
Copy link
Member Author

janos commented Nov 23, 2018

Thank you @justelad for a review and @zelig and @nonsense for comments. I have addressed your suggestions. Please check if they are ok, especially the metrics part.

swarm/shed/index.go Outdated Show resolved Hide resolved
@zelig
Copy link
Member

zelig commented Nov 25, 2018

address @justelad and @nonsense then we can submit to upstream

@janos
Copy link
Member Author

janos commented Nov 26, 2018

Submitted upstream ethereum/go-ethereum#18183.

@janos janos closed this Nov 26, 2018
@zelig zelig mentioned this pull request Nov 29, 2018
26 tasks
@frncmx frncmx deleted the shed branch January 22, 2019 09:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants