Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Fix/32/pr ports from go-ipfs to go-mfs #49

Merged
merged 6 commits into from
Jan 8, 2019

Conversation

nitishm
Copy link

@nitishm nitishm commented Dec 25, 2018

#32 is partially completed by this PR.

Personally this doesn't seem like the best way to do things, by forcing updateChildEntry even on stateFlushed but currently that is the only way to unblock waitPub which is blocking in the FlushPath call.

A better solution will be based on the completion of #47 which should eliminate waitPub

Fixes #50.
Fixes ipfs/kubo#4514.

@nitishm nitishm requested a review from schomatis as a code owner December 25, 2018 16:32
@schomatis
Copy link
Contributor

Agreed, let's wait on #47.

@schomatis schomatis added the status/blocked Unable to be worked further until needs are met label Dec 25, 2018
@nitishm
Copy link
Author

nitishm commented Dec 25, 2018

A better idea would be a new state (stateCreated).
Even better idea would be to store the state somewhere other than fd to make it work with the repub (maybe in the File struct ?). Will have to check where else can we store the state.

@schomatis
Copy link
Contributor

A better idea would be a new state (stateCreated).

SGTM

Even better idea would be to store the state somewhere other than fd to make it work with the repub (maybe in the File struct ?). Will have to check where else can we store the state.

It makes sense, but we only allow one RW file descriptor (and potentially many RO descriptors), so in a way we are centralizing the file state there.

@nitishm
Copy link
Author

nitishm commented Dec 25, 2018

One of the test cases randomly fails with a 0 being passed to Int63n.
Any ideas why this happens @schomatis ?

panic: invalid argument to Int63n
goroutine 37 [running]:
math/rand.(*Rand).Int63n(0xc00009a210, 0x0, 0xbe73c0)
	/home/travis/.gimme/versions/go/src/math/rand/rand.go:111 +0x17d
math/rand.Int63n(...)
	/home/travis/.gimme/versions/go/src/math/rand/rand.go:319
github.com/ipfs/go-mfs.actorWriteFile(0xc0001c4c00, 0x5, 0x3)
	/home/travis/gopath/src/github.com/ipfs/go-mfs/mfs_test.go:643 +0x1af
github.com/ipfs/go-mfs.testActor(0xc00003e6d0, 0x32, 0xc0000329c0)
	/home/travis/gopath/src/github.com/ipfs/go-mfs/mfs_test.go:698 +0x201
created by github.com/ipfs/go-mfs.TestMfsStress

See Jenkins results https://travis-ci.org/ipfs/go-mfs/builds/472151928?utm_source=github_status&utm_medium=notification

@schomatis
Copy link
Contributor

The function seems to panic if it gets an n <= 0 (https://golang.org/pkg/math/rand/#Int63n), are we correctly reporting the created file size?

s, err := fi.Size()

Also, TestMfsStress is a heavily concurrent function, so any errors there are normally linked to a possible data race.

@nitishm
Copy link
Author

nitishm commented Dec 25, 2018

Yeah that's where I am stuck. Need to figure out why my changes would affect the file creation since almost all of them are concerned only with RW flags for the fd's. Bummer.

Will try to dig in a little further. Gotta spend some time looking at the test helper functions.

fd.go Outdated Show resolved Hide resolved
Copy link
Author

@nitishm nitishm left a comment

Choose a reason for hiding this comment

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

The function seems to panic if it gets an n <= 0 (https://golang.org/pkg/math/rand/#Int63n), are we correctly reporting the created file size?

go-mfs/mfs_test.go

Line 633 in 64e7cdb
s, err := fi.Size()

Also, TestMfsStress is a heavily concurrent function, so any errors there are normally linked to a possible data race.

What does this method do ? I am completely lost here.

https://github.com/nitishm/go-mfs/blob/3191540a82040b2d124ab561fcfd971e590819b6/mfs_test.go#L506-L533

fd.go Outdated Show resolved Hide resolved
@nitishm
Copy link
Author

nitishm commented Dec 26, 2018

The failure in TestMfsStress seems to be surfacing, at random, due to this issue #50.

@schomatis If the proposal makes sense I will create a PR for it and get it into mfs_test.go ASAP.

@schomatis
Copy link
Contributor

The failure in TestMfsStress seems to be surfacing, at random

That entails that this should also happen in master, which I haven't seen before, so before merging anything please create first a PoC PR when TestMfsStress fails without the logic added in this PR, that is, feel free to increase the number of goroutines and iterations in the test to values that can trigger this panic.

fd.go Show resolved Hide resolved
@schomatis
Copy link
Contributor

I'm not sure this PR is sitting on top of all the commits of #45, @nitishm could you check that?

@schomatis
Copy link
Contributor

#45 is merged now.

@nitishm
Copy link
Author

nitishm commented Dec 27, 2018

#45 is merged now.

In that case do I need to do anything ?

@schomatis
Copy link
Contributor

In that case do I need to do anything ?

No, just rebase to (the new) master if needed.

@nitishm
Copy link
Author

nitishm commented Dec 27, 2018

@schomatis Do you think we should get this in even without #47 being merged in since it bypasses the waitPub issue in any case ?

@nitishm
Copy link
Author

nitishm commented Dec 27, 2018

The only commit remaining from #32 is ipfs/kubo@f4dc9a4

@schomatis
Copy link
Contributor

@schomatis Do you think we should get this in even without #47 being merged in since it bypasses the waitPub issue in any case ?

Yes, if it makes sense on its own, we can unblock it.

The only commit remaining from #32 is ipfs/go-ipfs@f4dc9a4

We should definitely add this since it was testing the new modifications (at that time) and it was failing.

@schomatis schomatis removed the status/blocked Unable to be worked further until needs are met label Dec 27, 2018
@nitishm
Copy link
Author

nitishm commented Dec 27, 2018

We should definitely add this since it was testing the new modifications (at that time) and it was failing.

I merged those changes and all the tests pass, but it takes close to 500 seconds to run the 5000 iterations in TestConcurrentWriteAndFlush (with the new changes to writeFile, with the Truncate, Seek, etc ops added).

You think I should bring the iterations down to a 100 (0.063s) or 500 (0.881s) or 1000 (2.710s) vs 5000 (491.51s) maybe ?

I will add the commit once I know what to set the value to.

@schomatis
Copy link
Contributor

ouch, that's a lot, yes, let's keep it under 1-2 seconds, it's a bit suspicious that a single writeFile iteration takes 100 ms, are we waiting for the republisher or something like that?

Also, your scale doesn't seem very proportionate, 5000 iterations costs 500 seconds, but dropping it to 1000 lowers it to 2.7? and 500 (1/10th of the original) is run in less than a second?

All that being said, if the tests are passing we're already on (a very good) track, we'll just have to fine tune some things. I'll give the PR a proper review in the following days.

@schomatis
Copy link
Contributor

@nitishm Not very important, but could you try rebasing this PR onto the current master when you get a chance, I think that will help with the review, commit 1bbc52d has already been merged but it's still showing up in this PR (I guess because there are other commits after that not included here), getting everything in sync should clear the diff.

nmalhotra added 4 commits December 30, 2018 18:25
Pulled changes from ipfs/kubo#4517, on top of, ipfs#45.
Change added to unblock the `waitPub()` call. With the elimination of
stateSync cause a `updateChildEntry` to happen for `stateFlushed` as
well, causing it to propogate upwards to the parent(s) [fullSync] and
force a publish to happen, hence unblocking `waitPub`.
Added a new state for a freshly opened `fileDescriptor`.
In `flushUp` only bubble up update if state is either `stateDirty` or
`stateCreated`. `stateFlushed` should prevent bubble up.
Add 1 to rand.Intn() to prevent 0 size reader for being created.
During `flushUp()` of an un-flushed fd, be it with freshly created
or modified content, always update the file descriptors inode,
with the contents of the node.
nmalhotra added 2 commits December 30, 2018 18:25
Pull in commit from go-ipfs commit
ipfs/kubo@f4dc9a4
Dropped nloop to 500 (from 1000) in `TestConcurrentWriteAndFlush` to
reduce testing time.

All unittests pass.
@nitishm
Copy link
Author

nitishm commented Dec 30, 2018

@nitishm Not very important, but could you try rebasing this PR onto the current master when you get a chance, I think that will help with the review, commit 1bbc52d has already been merged but it's still showing up in this PR (I guess because there are other commits after that not included here), getting everything in sync should clear the diff.

@schomatis I did something. I don't know if I did the right thing, to do the rebase.

Does this sound right ? From nitishm/go-mfs repo I did -

1. git remote add go-mfs https://github.com/ipfs/go-mfs.git
2. git fetch go-mfs
3. git rebase go-mfs/master
4. git pull
5. git push

Does that sound right ? I dont have too much experience with dealing with forks and keeping them up to date.

@schomatis
Copy link
Contributor

git rebase --onto master 2642dbf^ 40c7e34 -i   # <-- Check that those are actually the commits you want!
git checkout -B fix/32/pr-ports
git log master.. # Check the log before overwriting the PR.
git push # -f (to overwrite.)

You'd normally want to use the --onto option when rebasing (otherwise git does some weird stuff like rebasing things on top of themselves), I usually use the interactive -i option to check that git is about to do what I expect it to.

@nitishm
Copy link
Author

nitishm commented Dec 31, 2018

So do I need to undo any of this or does it look right to you ?

@nitishm
Copy link
Author

nitishm commented Dec 31, 2018

git rebase --onto master 2642dbf^ 40c7e34 -i   # <-- Check that those are actually the commits you want!
git checkout -B fix/32/pr-ports
git log master.. # Check the log before overwriting the PR.
git push # -f (to overwrite.)

You'd normally want to use the --onto option when rebasing (otherwise git does some weird stuff like rebasing things on top of themselves), I usually use the interactive -i option to check that git is about to do what I expect it to.

I managed to undo my last merge (hurray), and to rebased to the upstream/master. But it seems like my branch is already up to date with all the commits (atleast I can find 1bbc52d in my git log).

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Niiiiice. I'll leave the PR open for a couple of days in case @Stebalien wants to take a look at it (since we've been applying his code), particularly TestConcurrentWrites, since that patch came from a WIP PR that may have been unfinished.

@nitishm
Copy link
Author

nitishm commented Jan 2, 2019

SGTM ! @schomatis Got anything else you want me to look at, apart from the go-ipfs docs ?

@schomatis
Copy link
Contributor

Nothing important, we should prioritize the docs, they are also a great way for you to know what's out there in terms of the code base and to find other components you may be interested in.

@schomatis schomatis requested a review from Stebalien January 5, 2019 23:12
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.

@Stebalien
Copy link
Member

since that patch came from a WIP PR that may have been unfinished.

The WIP part was that I couldn't fix ipfs/kubo#4519.

@Stebalien Stebalien merged commit 1989254 into ipfs:master Jan 8, 2019
@schomatis
Copy link
Contributor

Great work @nitishm!! 🎉

@nitishm
Copy link
Author

nitishm commented Jan 8, 2019

Thanks for all the help @schomatis . I thoroughly appreciate it !

@Stebalien
Copy link
Member

Yes, thank you both!

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.

None yet

3 participants