Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add disable_repinning cluster option #387

Merged
merged 2 commits into from
Apr 24, 2018
Merged

Conversation

s1na
Copy link
Collaborator

@s1na s1na commented Apr 22, 2018

This PR adds the option disable_repinning along with its default value, and test values. repinFromPeer returns if this option is true.

I'm having problems with running the tests to ensure everything is passing. I get ipfscluster_test.go:69: bootstrap unsuccessful, does anyone know what might be the cause?

@GitCop
Copy link

GitCop commented Apr 22, 2018

There were the following issues with your Pull Request

  • Commit: 19b6208
  • Invalid signoff. Commit message must end with
    License: MIT
    Signed-off-by:

Guidelines are available at https://github.com/ipfs/ipfs-cluster/blob/master/contribute.md


This message was auto-generated by https://gitcop.com

@ghost ghost assigned s1na Apr 22, 2018
@ghost ghost added the status/in-progress In progress label Apr 22, 2018
License: MIT
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@s1na s1na force-pushed the fix/config/disable-repinnings branch from 19b6208 to 0954c6d Compare April 22, 2018 16:42
@coveralls
Copy link

coveralls commented Apr 22, 2018

Coverage Status

Coverage increased (+0.1%) to 67.348% when pulling 03cc809 on fix/config/disable-repinnings into da0915a on master.

@ZenGround0
Copy link
Collaborator

Not sure what's causing your tests to fail locally but everything is passing on CI. If you follow GitCop's instructions for a valid signoff then this LGTM.

@ZenGround0 ZenGround0 requested a review from hsanjuan April 23, 2018 00:57
@s1na
Copy link
Collaborator Author

s1na commented Apr 23, 2018

@ZenGround0 Okay thanks, I have fixed the commit's signature.

@hsanjuan
Copy link
Collaborator

hsanjuan commented Apr 23, 2018

@s1na this looks good to me, do you think you can add a test?

i.e. this test https://github.com/ipfs/ipfs-cluster/blob/master/ipfscluster_test.go#L1259 checks that re-allocation happens. Something similar (probably simpler) should check that re-allocation doesn't happen (setting DisableRepinning) at the beginning.

Let us know if you find it too difficult though.

cluster.go Outdated
@@ -397,6 +397,10 @@ func (c *Cluster) watchPeers() {

// find all Cids pinned to a given peer and triggers re-pins on them.
func (c *Cluster) repinFromPeer(p peer.ID) {
if c.config.DisableRepinning {
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add a logger.Warn("repinning is disabled. Will not re-allocate cids from %s")

* Test case creates a bunch of clusters, assigns a pin with replica factor
of n-1 to them, and removes one of the peers randomly. It then tests
to check that the number of clusters pinning the cid is n-2.
* Add warn log to let user know that due to disable_repinning option,
the cluster won't attempt to re-assign the pin.

License: MIT
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
@s1na s1na changed the title [WIP] Add disable_repinning cluster option Add disable_repinning cluster option Apr 23, 2018
@hsanjuan hsanjuan merged commit a5710dd into master Apr 24, 2018
@ghost ghost removed the status/in-progress In progress label Apr 24, 2018
@hsanjuan hsanjuan deleted the fix/config/disable-repinnings branch April 24, 2018 07:25
@hsanjuan
Copy link
Collaborator

THANKS!

@hsanjuan hsanjuan mentioned this pull request Apr 24, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants