-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
PR ethereum/go-ethereum#19215 was opened by mistake on ethereum, which can be reused if this PR is accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it not make sense to split this PR and extract the Has/shed related changes?
Yes, I will create a new PR, Has is pretty isolated change. |
@@ -135,13 +135,10 @@ func dbImport(ctx *cli.Context) { | |||
log.Info(fmt.Sprintf("successfully imported %d chunks", count)) | |||
} | |||
|
|||
func openLDBStore(path string, basekey []byte) (*storage.LDBStore, error) { | |||
func openLDBStore(path string, basekey []byte) (*localstore.DB, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this not be in the localstore pkg as one of the constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am not sure that we need it. It only checks if goleveldb data is present in a directory. Is it really necessary to have that check for export and import?
I am blocking this PR as it presents much more problems then improvements. From gitter https://gitter.im/ethersphere/hq?at=5c9a376081b15c5e4b8ca43a:
Please, do not review this PR until an alternative solution is presented. |
if peerPO > po { | ||
mode = chunk.ModePutRequest | ||
} else { | ||
mode = chunk.ModePutSync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justelad
- no its not generating more redundancy the rest i dont understand
- it is easy to construct test cases. this is about correctness
swarm/network/stream/delivery.go
Outdated
} else { | ||
depth := d.kad.NeighbourhoodDepth() | ||
// is the chunk within our area of responsibility? | ||
if po < depth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if po >= depth || peerPO < po {
mode = chunk.ModePutSync
} else {
mode = chunk.ModePutRequest
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is more compact representation. I thought that the current one was easier to reason about, but this is good with a comment, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is good but you got the comparison wrong, note
po >= depth
-> sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Viktor.
Thanks for reviews. @holisticode I have set this PR ready for review, again. Before this gets merged, maybe we should decide about the migration PR that @justelad is working on. If this PR is merged, and released without the migration part, users will not have an option to keep their existing data. |
// create a pull syncing triggers used by SubscribePull function | ||
db.pullTriggers = make(map[uint8][]chan struct{}) | ||
// push index contains as yet unsynced chunks | ||
db.pushIndex, err = db.shed.NewIndex("StoredTimestamp|Hash->nil", shed.IndexFuncs{ | ||
db.pushIndex, err = db.shed.NewIndex("StoreTimestamp|Hash->Tags", shed.IndexFuncs{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zelig @justelad do we need to revert this index not to have Tags and remove tags from shed Item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janos we definitely need a way to persist tags and I think that this is the correct place to do so. If anything, tags should be removed from chunk.Chunk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk.Chunk
does not have reference to Tags, currently. Are you referring to shed.Item
or some other code?
@@ -141,13 +141,13 @@ func netStoreAndDeliveryWithAddr(ctx *adapters.ServiceContext, bucket *sync.Map, | |||
|
|||
cleanup := func() { | |||
netStore.Close() | |||
os.RemoveAll(datadir) | |||
localStoreCleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not localStore.Close()
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netStore.Close() is calling localstore.Close()
teardown := func() { | ||
streamer.Close() | ||
intervalsStore.Close() | ||
netStore.Close() | ||
removeDataDir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localStore.Close()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netStore.Close() is calling localstore.Close()
@janos I think we can close this. I think we should consider this merged, since it is in the |
Yes, thanks, Anton. Closing this PR since it is merged into https://github.com/ethersphere/go-ethereum/tree/swarm-rather-stable branch. |
This PR replaces LDBStore with new localstore DB in all parts of the code.
Work needed to be done is migration from legacy data store #1297 and logging and tracing in localstore package. Beside this missing parts, this PR should provide a functional swarm node starting from a blank database.