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.

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

Make repo gc call CollectGarbage on datastore #4578

Merged
merged 2 commits into from
Feb 4, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Jan 13, 2018

Depends on ipfs/go-ds-measure#13

I'm not sure how to approach adding tests for this. Badger does implement this interface and it does get called, though due to it's GC weirdness I'm unable to come up with scenario in which it will actually free-up some space.

(Badger was updated to current master, doesn't seem to break anything from my testing)


err = gds.CollectGarbage()
if err != nil {
output <- Result{Error: err}
Copy link
Member

Choose a reason for hiding this comment

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

This can deadlock if the context has been canceled and output is full. However, we already have this bug (above) so feel free to fix it separately. See #4593.

@@ -366,6 +369,21 @@ func BadgerdsDatastoreConfig(params map[string]interface{}) (DatastoreConfig, er
}
}

vls, ok := params["vlogFileSize"]
if !ok {
c.vlogFileSize = 1 << 30
Copy link
Member

Choose a reason for hiding this comment

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

Why? Could you add a comment explaining this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the default size, will add a comment

@magik6k magik6k force-pushed the feat/datastore-gc branch 3 times, most recently from 8c25e64 to 0e58b0c Compare January 25, 2018 17:54
@magik6k
Copy link
Member Author

magik6k commented Jan 25, 2018

This is currently blocked by go-datastore being updated in go-ds-measure and not here: ipfs/go-ds-measure@d287c4e#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

@Stebalien
Copy link
Member

Blocked on #4610.

@Stebalien
Copy link
Member

Unblocked, needs rebase.

@magik6k
Copy link
Member Author

magik6k commented Jan 31, 2018

Rebased

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. Eventually, the we should probably expose this through the blockstore (as GC is really happening at the blockstore level) but we can do that later.

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

What is the new vlogFileSize parameter and how does it relate to calling CollectGarabge on the datastores?

If this is not related to the purpose of this P.R. could we separate it out into a separate commit?

If it is related could we mention it in the commit message so someone not familiar with the Badger Datastore code knows how it relates?

@magik6k
Copy link
Member Author

magik6k commented Feb 1, 2018

What is the new vlogFileSize parameter and how does it relate to calling CollectGarabge on the datastores?

Badger is currently the only DS that implements GC, the minimum reclaimable space is defined as max(10mb, vlogFileSize*gcTreshold) per vlog. When I tried to create tests for this, I needed this parameter, so the tests wouldn't need to create multi-gb datastore to trigger gc.

If this is not related to the purpose of this P.R. could we separate it out into a separate commit?

Done

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

LGTM

@whyrusleeping
Copy link
Member

@magik6k sorry, need a rebase here.

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@whyrusleeping whyrusleeping merged commit 52301ce into master Feb 4, 2018
@whyrusleeping whyrusleeping deleted the feat/datastore-gc branch February 4, 2018 07:38
@ghost ghost removed the status/in-progress In progress label Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants