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

DB Store Abstractions#993

Closed
janos wants to merge 17 commits intomasterfrom
shed
Closed

DB Store Abstractions#993
janos wants to merge 17 commits intomasterfrom
shed

Conversation

@janos
Copy link
Copy Markdown
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
Comment thread swarm/shed/db.go Outdated
Comment thread swarm/shed/index_mock.go Outdated
Comment thread swarm/shed/index.go
// 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
Copy Markdown
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
Copy Markdown
Contributor

@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 🙏

Comment thread swarm/shed/db.go Outdated
Comment thread swarm/shed/db.go Outdated
Comment thread swarm/shed/db.go Outdated
Comment thread swarm/shed/db.go Outdated
Comment thread swarm/shed/db.go
Comment thread swarm/shed/index.go Outdated
Comment thread swarm/shed/index.go
Comment thread swarm/shed/index.go Outdated
Comment thread swarm/shed/schema.go
Comment thread swarm/shed/schema.go Outdated
Comment thread swarm/shed/index.go
@acud
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread swarm/shed/index.go Outdated
@zelig
Copy link
Copy Markdown
Member

zelig commented Nov 25, 2018

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

@janos
Copy link
Copy Markdown
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