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

feat: add support for batch puts #238

Closed
wants to merge 1 commit into from
Closed

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented Oct 9, 2019

This PR add a new method to put Blocks in batch to the datastore.

supports: ipfs-inactive/js-ipfs-unixfs-importer#38

@hugomrdias hugomrdias requested review from vmx and achingbrain October 9, 2019 09:37
@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #238 into master will decrease coverage by 1.68%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   86.84%   85.16%   -1.69%     
==========================================
  Files           2        2              
  Lines         152      155       +3     
==========================================
  Hits          132      132              
- Misses         20       23       +3
Impacted Files Coverage Δ
src/index.js 83.45% <80%> (-1.85%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1312244...b928703. Read the comment docs.

@achingbrain
Copy link
Member

Could you expand on the use case for this a little? My impression of the API for this module was that it abstracts interactions with the block store, encapsulating node serialization, etc so it seems weird to be passing blocks directly into it.

It also already has putMany (though it's essentially serial as implemented so needs improving).

@hugomrdias
Copy link
Member Author

hugomrdias commented Oct 10, 2019

Could you expand on the use case for this a little? My impression of the API for this module was that it abstracts interactions with the block store, encapsulating node serialization, etc so it seems weird to be passing blocks directly into it.

Yep for me it was weird to have blockstore inside IPLD, it shouldn't handle persistence only data formats. Also it made no sense to create another Array with map when we can send Blocks directly. This is related to performance work in unixfs so it made sense.

It also already has putMany (though it's essentially serial as implemented so needs improving).

Yes putMany its.. well as you said needs improving.


putBatch (blocks, options) {
Copy link
Member

Choose a reason for hiding this comment

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

This should take IPLD nodes and not blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Which is what putMany does.

@vmx
Copy link
Member

vmx commented Oct 29, 2019

The problem this PR is addressing will be solved on a different layer. For more information see ipfs-inactive/js-ipfs-unixfs-importer#38.

@vmx vmx closed this Oct 29, 2019
@daviddias daviddias deleted the feat/batch-put branch October 29, 2019 16:25
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