Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Fix #27: Remove batching from importers #41

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Fix #27: Remove batching from importers #41

merged 2 commits into from
Oct 26, 2018

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Oct 19, 2018 •
edited
Loading

A batching DAG service is forced onto the
users of the importers, but they could just wrap
the given DAGService in the batching one to get the same
functionality (remembering to Close it at the end of the
proccess).

As detailed in #27, the importers should not be making choices
about what DAGService is the right one to use and wrapping the
given one.

This change requires wrapping the DAGService in go-ipfs into
ipld.Batch. and closing it when Finishing the adding process.

@hsanjuan hsanjuan self-assigned this Oct 19, 2018
@ghost ghost added the status/in-progress In progress label Oct 19, 2018
@hsanjuan
Copy link
Contributor Author

I went ahead and removed the ipld.Batch from here as discussed in #27, at least so I can move forward on cluster things which are broken because of this without doing a whole un-batching workaround for the moment.

Discussion pending of whether this is the best approach (knowing that it is breaking and requires a small change in go-ipfs), or whether in turn we want to make the importer be able to disable batching but leave it enable by default (which would make it compatible, but ihmo just adds more code that shouldn't belong in this module).

schomatis
schomatis approved these changes Oct 19, 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, this definitely makes more sense to me but someone else should weigh in on the impact it might have on the go-ipfs side.

magik6k
magik6k approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

LGTM, is there a pr for this on the go-ipfs side?

@hsanjuan
Copy link
Contributor Author

I'll take care of making a PR on go-ipfs. :)

@hsanjuan hsanjuan mentioned this pull request Oct 22, 2018
Merged
hsanjuan added a commit to ipfs/kubo that referenced this pull request Oct 22, 2018
@hsanjuan
Explicitally use Batch after removing it from importers
aa3f185 
See:

* ipfs/go-unixfs#41
* ipfs/go-ipld-format#46

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@hsanjuan hsanjuan mentioned this pull request Oct 22, 2018
Merged
6 tasks
schomatis
schomatis suggested changes 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.

The latest modifications to the PR are now rewriting paths with Gx, why?

@hsanjuan
Copy link
Contributor Author

sorry, my mess. Fixed @schomatis

@hsanjuan hsanjuan changed the title [DNM] Fix #27: Remove batching from importers Fix #27: Remove batching from importers Oct 23, 2018
@hsanjuan
Copy link
Contributor Author

So, if I have approval, I will merge this only when the related changes in go-ipld-format and go-ipfs come through, and all the dependencies are aligned (and rebased to master if needed). For the moment it can stay like this.

@schomatis schomatis added status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress labels Oct 24, 2018
@hsanjuan
Fix #27: Remove batching from importers
6078141 
A batching DAG service is forced onto the
users of the importers, but they could just wrap
the given DAGSerice in the batching one to get the same
functionality (remembering to Close it at the end of the
proccess).

As detailed in #27, the importers should not be making choices
about what DAGService is the right one to use and wrapping the
given one.

This change requires wrapping the DAGService in go-ipfs into
ipld.Batch. and closing it when Finishing the adding process.
@hsanjuan
gx publish 1.1.16
2d94c3b
@ghost ghost added the status/in-progress In progress label Oct 26, 2018
@hsanjuan hsanjuan merged commit e2784eb into master Oct 26, 2018
@ghost ghost removed the status/in-progress In progress label Oct 26, 2018
@hsanjuan
Copy link
Contributor Author

bubbling now. tests good manually.

@hsanjuan hsanjuan deleted the fix/27-remove-batching branch October 26, 2018 14:04
@schomatis schomatis removed the status/blocked Unable to be worked further until needs are met label Oct 29, 2018
Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 25, 2023
@hsanjuan
Merge pull request ipfs/go-unixfs#41 from ipfs/fix/27-remove-batching
0aa1b1a 
Fix ipfs#27: Remove batching from importers

This commit was moved from ipfs/go-unixfs@e2784eb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schomatis schomatis schomatis approved these changes

@magik6k magik6k magik6k approved these changes

@Stebalien Stebalien Awaiting requested review from Stebalien

Assignees

@hsanjuan hsanjuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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