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

Explicitally use BufferedDAG after removing Batch from importers #5626

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Explicitally use BufferedDAG after removing Batch from importers #5626

merged 2 commits into from
Oct 30, 2018

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Oct 22, 2018 •
edited
Loading

@hsanjuan hsanjuan requested a review from Kubuxu as a code owner October 22, 2018 14:35
This was referenced Oct 22, 2018
Merged
Merged
Merged
Merged
Merged
Merged
@ghost ghost assigned hsanjuan Oct 22, 2018
@ghost ghost added the status/in-progress In progress label Oct 22, 2018
schomatis
schomatis reviewed Oct 22, 2018
View reviewed changes
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Could you split the Gx update in a separate commit to make it easier to see the changes to the logic?

@hsanjuan
Copy link
Contributor Author

@schomatis done.

hsanjuan
hsanjuan commented Oct 26, 2018
View reviewed changes
core/coreunix/add.go Outdated
@@ -47,13 +47,31 @@ type Object struct {
Size string
}

// A batch-adding dag service for use with importers (and only for that)
// Always Commit() after adding.
type batching struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we make the DagBuilderHelper take separately an ipld.NodeAdder and an ipld.NodeGetter (or an adder + a separate DAGService) rather than just a DAGService, we need to wrap Batch in a DAGService, as it seems trickle uses Get.

Copy link
Member

Choose a reason for hiding this comment

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

If it uses get as I think it does, we probably need to commit before calling get. Let's go with plan b and make Batch implement DAGService (flushing on get). (unless you can think of a better solution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien yeah i'll do that to be on the safe side.

@hsanjuan hsanjuan changed the title Explicitally use Batch after removing it from importers Explicitally use BufferedDAG after removing Batch from importers Oct 27, 2018
@hsanjuan
Copy link
Contributor Author

@Stebalien @schomatis this ready using BufferedDAG with all bubbled.

@hsanjuan hsanjuan mentioned this pull request Oct 27, 2018
Closed
@hsanjuan
Copy link
Contributor Author

I'm not sure test failures have anything to do with this

schomatis
schomatis approved these changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

LGTM

@hsanjuan
Bubble deps
ef7234d 
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan
Add batching when Adding.
8709e55 
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
magik6k
magik6k approved these changes Oct 30, 2018
View reviewed changes
core/coreunix/add.go
@@ -49,11 +49,14 @@ type Object struct {

// NewAdder Returns a new Adder used for a file add operation.
func NewAdder(ctx context.Context, p pin.Pinner, bs bstore.GCBlockstore, ds ipld.DAGService) (*Adder, error) {
bufferedDS := ipld.NewBufferedDAG(ctx, ds)
Copy link
Member

Choose a reason for hiding this comment

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

I'll probably refactor this to happen on the coreapi layer (next to offline/sessions stuff), but this is good enough for now

@Stebalien Stebalien merged commit 0182659 into master Oct 30, 2018
@ghost ghost removed the status/in-progress In progress label Oct 30, 2018
@Stebalien Stebalien deleted the feat/impoter-batching branch October 30, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schomatis schomatis schomatis approved these changes

@magik6k magik6k magik6k approved these changes

@Stebalien Stebalien Awaiting requested review from Stebalien

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@hsanjuan hsanjuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hsanjuan @Stebalien @magik6k @schomatis