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.

Sign up for GitHub

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

Jump to bottom

Support Asynchronous Datastores #6785

Merged
merged 1 commit into from
Dec 19, 2019
Merged

Support Asynchronous Datastores #6785

merged 1 commit into from
Dec 19, 2019

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Dec 5, 2019 •
edited
Loading

As part of #6775, we need to add Sync calls into the appropriate places in go-ipfs (and its dependencies).

The places that were deemed important to have Sync calls include:

  • Pinning: Any time the pin root is flushed all the puts it relies on should have completed
  • Adding: After an add operation is completed the data should be synced
  • MFS: Anytime the root is modified
  • IPNS: When an update is submitted (before any network calls are made)

@aschmahmann aschmahmann requested review from Stebalien and dirkmc and removed request for lanzafame December 5, 2019 18:42
@aschmahmann aschmahmann force-pushed the feat/async-ds branch 5 times, most recently from a8cfdba to 5e49b9d Compare December 5, 2019 22:59
Stebalien
Stebalien reviewed Dec 6, 2019
View reviewed changes
core/node/core.go Outdated Show resolved Hide resolved
@aschmahmann aschmahmann force-pushed the feat/async-ds branch 3 times, most recently from 0c867ba to a0b85d3 Compare December 6, 2019 16:30
@aschmahmann aschmahmann mentioned this pull request Dec 9, 2019
Merged
@aschmahmann aschmahmann force-pushed the feat/async-ds branch 2 times, most recently from 76c7c46 to 7e75734 Compare December 16, 2019 19:03
@aschmahmann aschmahmann marked this pull request as ready for review December 17, 2019 07:08
Stebalien
Stebalien requested changes Dec 18, 2019
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I think there has been a miscommunication or misunderstanding somewhere. Let's start over with a design proposal in #6775 that specifies the exact sequence of sync calls we need, where, and why.

core/coreapi/unixfs.go Outdated Show resolved Hide resolved
core/coreunix/add.go Outdated Show resolved Hide resolved
core/node/core.go Show resolved Hide resolved
core/node/core.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

Ok, now that I remember how the pinner is calling Sync, this makes more sense. However, the calls to Sync(cid) are still wrong. Let's still write a proposed design with what/where/why we need to call sync.

@aschmahmann
Copy link
Contributor Author

However, the calls to Sync(cid) are still wrong

Sure, we can just call Sync on bstore.BlockPrefix it's the correct thing to do (note that all the current implementations work fine even with this being done incorrectly).

@aschmahmann aschmahmann force-pushed the feat/async-ds branch 2 times, most recently from ac5af0b to 328aac8 Compare December 18, 2019 14:47
aschmahmann
aschmahmann commented Dec 18, 2019
View reviewed changes
core/node/core.go Outdated
pinning, err := pin.LoadPinner(repo.Datastore(), ds, internalDag)
rootDS := repo.Datastore()

syncFn := func() error { return rootDS.Sync(blockstore.BlockPrefix) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason the pinner needs to sync the Filestore is there?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, yes. When pinning, we may reference blocks in the filestore.

Stebalien
Stebalien requested changes Dec 18, 2019
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM with one change.

core/node/core.go Outdated
pinning, err := pin.LoadPinner(repo.Datastore(), ds, internalDag)
rootDS := repo.Datastore()

syncFn := func() error { return rootDS.Sync(blockstore.BlockPrefix) }
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, yes. When pinning, we may reference blocks in the filestore.

@Stebalien Stebalien merged commit 3c95f65 into master Dec 19, 2019
@Stebalien Stebalien deleted the feat/async-ds branch December 19, 2019 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@dirkmc dirkmc dirkmc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aschmahmann @Stebalien @dirkmc