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

feat(importer): add rabin fingerprinting chunk algorithm #223

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

dordille
Copy link
Contributor

This is required to have feature parity with go-ipfs which supports rabin chunking algorithm.

Rabin chunker supports the following chunkerOptions, defaults mirror those in go-ipfs-chunker.

name type default
minChunkSize Integer avgChunkSize / 3
avgChunkSize Integer 262144
maxChunkSize Integer avgChunkSize + (avgChunkSize / 2)
window Integer 16
polynomial String 0x3DF305DFB2A805

This currently works as is, however the parameters polynomial and windowsize are not configurable by the rabin library used. This makes it difficult to compare the output the go-ipfs implementation, since both these parameters are set to different values there.

Could use some advice on testing, the nature of the rabin chunking makes it difficult to test things like expected block sizes, and number of blocks.

These changes are required for ipfs/js-ipfs/issues/1283

This is required to have feature parity with go-ipfs which supports rabin chunking algorithm.
Rabin chunker supports the following `chunkerOptions`
minChunkSize: {integer}
avgChunkSize: {integer}
maxChunkSize: {integer}
polynomial:   {string}
window:       {integer}
Rabin chunker uses the same defaults defined by go-ipfs-chunker.

Supports ipfs/js-ipfs#1283

License: MIT
Signed-off-by: Dan Ordille <[email protected]>
@vmx
Copy link
Contributor

vmx commented Aug 6, 2018

@dordille I finally had a chance to review the code. It looks good so far.

Though the code coverage bot reports a decrease. It took me a while to figure out why, because I saw that you added tests. The problem is that your tests are not actually run, so please make them run and have another look at the coverage.

For further testing, the Go implementation could be an inspiration.

License: MIT
Signed-off-by: Dan Ordille <[email protected]>
@achingbrain
Copy link
Collaborator

This looks good. I think you need to add one more test to cover the situation when you don't specify a minChunksize to the chunker.

@dordille
Copy link
Contributor Author

dordille commented Aug 7, 2018

Is there a way to force a jenkins build without pushing a commit, I don't think the current failure is a result of my changes.

@vmx
Copy link
Contributor

vmx commented Aug 7, 2018

@dordille I think only people with merge permissions are able to retrigger a build. Sadly there's currently env related issues (workers running out of disk space). I'll make sure to have a clean run. If it should turn out that the failure is due to your code, I'll let you know. Sorry for the inconvenience.

@achingbrain achingbrain merged commit be07987 into ipfs-inactive:master Aug 8, 2018
@achingbrain
Copy link
Collaborator

Jenkins, ugh. See ipfs-inactive/dev-team-enablement#70 for the reasons behind the disk space thing.

Anyway this has been merged & published as [email protected].

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.

3 participants