Skip to content

Remove GC timeout, fix GC tests - #3494

Merged
whyrusleeping merged 2 commits into
masterfrom
fix/gc-timeout
Dec 16, 2016
Merged

Remove GC timeout, fix GC tests#3494
whyrusleeping merged 2 commits into
masterfrom
fix/gc-timeout

Conversation

@ghost

@ghost ghost commented Dec 9, 2016

Copy link
Copy Markdown

Closes #3436 #2129

commit a9b8c8b
Author: Lars Gierth larsg@systemli.org
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 <larsg@systemli.org>

commit 8f0c7ee
Author: Lars Gierth larsg@systemli.org
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 <larsg@systemli.org>

commit 49802eb
Author: Lars Gierth larsg@systemli.org
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 <larsg@systemli.org>

@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

ghost commented Dec 9, 2016

Copy link
Copy Markdown
Author

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

@Kubuxu

Kubuxu commented Dec 9, 2016

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@ghost

ghost commented Dec 10, 2016

Copy link
Copy Markdown
Author

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

@Kubuxu

Kubuxu commented Dec 10, 2016

Copy link
Copy Markdown
Member

I don't think it will be easy.

@whyrusleeping

Copy link
Copy Markdown
Member

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

@Kubuxu

Kubuxu commented Dec 10, 2016

Copy link
Copy Markdown
Member

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

@whyrusleeping

Copy link
Copy Markdown
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 <larsg@systemli.org>
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 <larsg@systemli.org>
@whyrusleeping

Copy link
Copy Markdown
Member

@lgierth progress here?

@ghost
ghost force-pushed the fix/gc-timeout branch from 49802eb to 2836c64 Compare December 16, 2016 00:16
@ghost

ghost commented Dec 16, 2016

Copy link
Copy Markdown
Author

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

ghost commented Dec 16, 2016

Copy link
Copy Markdown
Author

OSX failure is #3496

@whyrusleeping whyrusleeping left a comment

Copy link
Copy Markdown
Member

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