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

Remove GC timeout, fix GC tests #3494

Merged
merged 2 commits into from
Dec 16, 2016
Merged

Remove GC timeout, fix GC tests #3494

merged 2 commits into from
Dec 16, 2016

Conversation

ghost
Copy link

@ghost ghost commented Dec 9, 2016

Closes #3436 #2129

commit a9b8c8b
Author: Lars Gierth [email protected]
Date: Fri Dec 9 17:26:53 2016 +0100

gc: remove wonky timeout

This timeout was:
- unneccessary
- based on a metric that doesn't make sense

License: MIT
Signed-off-by: Lars Gierth <[email protected]>

commit 8f0c7ee
Author: Lars Gierth [email protected]
Date: Fri Dec 9 17:28:31 2016 +0100

gc: remove unneccessary full repo scan

GetStorageUsage() is super expensive as it involves a full repo scan,
and it's already bad enough that we have do it once.

The call that's being removed here is purely for cosmetical purposes:
printing the number of bytes freed by the GC run. Let's drop it.

License: MIT
Signed-off-by: Lars Gierth <[email protected]>

commit 49802eb
Author: Lars Gierth [email protected]
Date: Fri Dec 9 18:04:55 2016 +0100

gc: fix and re-enable sharness tests

I'm bumping up the waiting time significantly.
I prefer a slightly slower test to no test at all.

License: MIT
Signed-off-by: Lars Gierth <[email protected]>

@ghost ghost added regression topic/repo Topic repo topic/test failure Topic test failure labels Dec 9, 2016
@ghost ghost added this to the ipfs 0.4.5 milestone Dec 9, 2016
@ghost ghost added the status/in-progress In progress label Dec 9, 2016
@ghost
Copy link
Author

ghost commented Dec 9, 2016

Oh my, it timed out on me right away again. Is there any way to avoid the sleep? sync(1)?

@Kubuxu
Copy link
Member

Kubuxu commented Dec 9, 2016

Sync won't matter as it does kernel cache to disk and problem here is go-ipfs process to kernel. I would just suggest bigger files and error margins.

@whyrusleeping
Copy link
Member

Yeah... i remember this test being really troublesome. Let me know if you need any help @lgierth

@ghost
Copy link
Author

ghost commented Dec 10, 2016

If we're trying to wait for IPFS flusing its writes, why not expose the state of that in the diag command?

@Kubuxu
Copy link
Member

Kubuxu commented Dec 10, 2016

I don't think it will be easy.

@whyrusleeping
Copy link
Member

@lgierth ipfs flushes its writes... what is the problem?

@Kubuxu
Copy link
Member

Kubuxu commented Dec 10, 2016

The delay it will take to flush the write is unspecified AFAIK.

@whyrusleeping
Copy link
Member

The commands shouldnt return before things get flushed... (unless you have no-flush set)

Lars Gierth added 2 commits December 13, 2016 14:50
This timeout was:
- unneccessary
- based on a metric that doesn't make sense

License: MIT
Signed-off-by: Lars Gierth <[email protected]>
GetStorageUsage() is super expensive as it involves a full repo scan,
and it's already bad enough that we have do it once.

The call that's being removed here is purely for cosmetical purposes:
printing the number of bytes freed by the GC run. Let's drop it.

License: MIT
Signed-off-by: Lars Gierth <[email protected]>
@whyrusleeping
Copy link
Member

@lgierth progress here?

@ghost
Copy link
Author

ghost commented Dec 16, 2016

This is good to go, I've split the stuff about tests out into the feat/corerepo-gc branch, since it turned into a full-blown rewrite of this GC stuff. :)

This one here now only removes the timeout context around gc.GC(), and the second repo scan after the GC run.

@ghost
Copy link
Author

ghost commented Dec 16, 2016

OSX failure is #3496

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM, now we can at least run gc...

@whyrusleeping whyrusleeping merged commit e2ba43c into master Dec 16, 2016
@whyrusleeping whyrusleeping deleted the fix/gc-timeout branch December 16, 2016 04:15
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Dec 16, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic GC times out
2 participants