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

Review go-ipfs PR 4517 and see what can be extracted here. #32

Closed
3 of 4 tasks
schomatis opened this issue Dec 13, 2018 · 16 comments
Closed
3 of 4 tasks

Review go-ipfs PR 4517 and see what can be extracted here. #32

schomatis opened this issue Dec 13, 2018 · 16 comments
Assignees
Labels
status/ready Ready to be worked

Comments

@schomatis
Copy link
Contributor

schomatis commented Dec 13, 2018

ipfs/kubo#4517
ipfs/kubo#4758

Originally blocked by a flush bug (ipfs/kubo#4758 (comment)) there are interesting ideas and useful code that can still be extracted here.

Pending to review:

@schomatis
Copy link
Contributor Author

@nitishm You may be able to tackle one of the commits in the list if you're interested.

@nitishm
Copy link

nitishm commented Dec 21, 2018

Sure. Just a code review ? Any special process to follow ? Or maybe I am misunderstanding completely and you want to pull these changes (manually?) into go-mfs ?

@schomatis
Copy link
Contributor Author

Yes, the second, but you can start with the first, trying to understand one of those commits and if you feel comfortable try to import it here (manually or not depends on how much has changed since then, but yes, it will probably need some manual tailoring).

@nitishm
Copy link

nitishm commented Dec 21, 2018

Sounds good. I will take a look at it. From a quick glance seems like many constructs have changed like,
File.Open() method changed from

func (fi *File) Open(flags Flags) (_ FileDescriptor, _retErr error) {

to

func (fi *File) Open(flags int, sync bool) (FileDescriptor, error) {

Will try my best and hit you up with questions if I have any.

However, this one seems simple enough to port over - ipfs/kubo@ed9a405

@nitishm
Copy link

nitishm commented Dec 21, 2018

Which makes me think we might want to pull the options.Flags{} struct from ipfs/kubo@41c554b which means it's best to go in the order of commits. I am getting the hang of this so I will attempt to start with the first one in line.

@nitishm
Copy link

nitishm commented Dec 21, 2018

@schomatis I will take ipfs/kubo@41c554b and ipfs/kubo@a5ed5f3 and create a PR soon. (you can assign these to me). Also should I open a new issue to track them ?

Prior to doing that, ipfs/kubo@41c554b has changes that will affect ipfs/go-ipfs module, specifically package go-ipfs/fuse/ipns. How do you make PRs across repos ?

@nitishm
Copy link

nitishm commented Dec 21, 2018

#46 Might get fixed if I manage to pull the changes from ipfs/kubo@41c554b

@nitishm
Copy link

nitishm commented Dec 21, 2018

@schomatis I have made the changes but cannot get all the tests to pass.
Can you explain what this test does ?

https://github.com/ipfs/go-mfs/blob/master/mfs_test.go#L792-L858

The test case never writes to the file, therefore never changing the state of the file.

The problem I am facing is with the changes in place, if an filedescriptor state is equal to stateFlushed (which is the default state when the fd is created in file.Open()) it will never call parent.updateChildEntry() which in turn will never publish using the repub, hence causing the test to fail.

(PS : You will have to see the commit diff at ipfs/kubo@41c554b for file.go -> File.Open() and fd.go -> filedescriptor.flushUp())

@schomatis
Copy link
Contributor Author

Can you explain what this test does ?

They basically add a lot of children (files) and the flush each one individually so the modifications (new files) are reflected in the MFS root which should match the hard-coded hash in the test.

The original PR never got merged due to a flush bug so this may be related to that. See ipfs/kubo#4758 (comment) if it helps (I should have pointed you to the reworked PR in the first place). This may not be an easy bug to figure out so do what you can, any additional information would be very useful.

@nitishm
Copy link

nitishm commented Dec 22, 2018

@schomatis Do you remember what result of all the republisher refactor through @Stebalien's commits in the ipfs/kubo#4758 (comment).

I can follow the commits and am ready to pull those changes, but before I do that I want to know if it even worked or not ? Seems like the last comment after commit was the Sharness test failing ?!

In any case I could try and pull those commits in and retry the tests. Just thought I would ask.

Also is there a doc that explains the publishing system ? I don't completely comprehend the mechanism. What exactly is a publish and why do we have a short and long interval (what are these Intervals for anyway ?) ? Shouldn't it just be an event driven model, triggered by the publish method.

@nitishm
Copy link

nitishm commented Dec 22, 2018

@schomatis The only way I can see us get through that test case for TestFlushing is if we treat both stateSynced and stateFlushed the exact same way.
I.E. Get the node, parent and name from the File->inode and call parent.updateChildEntry with this child{name, node}, since that is the only way we can unblock the WaitPublish() to prevent a permanent hang.

@Stebalien @whyrusleeping What do you guys think ? Should states Synced and Flushed be treated the same way in flushUp ?

@schomatis
Copy link
Contributor Author

I can follow the commits and am ready to pull those changes, but before I do that I want to know if it even worked or not ?

It didn't, that's why we were extracting commit by commit to check what works and can be useful (a year after its creation).

In any case I could try and pull those commits in and retry the tests. Just thought I would ask.

Yes.

Also is there a doc that explains the publishing system ? I don't completely comprehend the mechanism. What exactly is a publish and why do we have a short and long interval (what are these Intervals for anyway ?) ? Shouldn't it just be an event driven model, triggered by the publish method.

Regrettably, no. The last refactoring round should have made it a bit simpler to follow. Please extract what follows to the general documentation issue in ipfs/doc:


There are two different mechanism which are generally encompassed in the "flush" terminology, one is (what I'd like to call) the actual flush of propagating a node update all the way up to the MFS root, so its CID (contents) reflect that change, the second is what we do with the updated CID. Publish here means basically either save the CID to disk or make an IPNS record so anyone looking for /ipns/some-readable-name will now get the new CID, hence syncing (although I'm not sure I like this term here) /ipns/my-personal-filesystem with this new CID, so if I do something like ipfs cat /ipns/my-personal-filesystem/some-dir/the-modified-file it will output the new contents of the the-modified-file.


The motivation behind the timers in now more closely reflected in the code comments, take a look and raise an issue if something doesn't look right. Also, I have PR #47 in the pipeline which would help to simplify things some more.

So, rereading my comment in ipfs/kubo#4758 (comment) (where I was mentioning a lot of methods I didn't know what they did at the time) the original problem seemed to be:

the fact that the MFS root is marked to be published but it isn't until another (different) file (node) is flushed, handling it in an indirect fashion.

(Although the simplifications in the PR pipeline should also help.)

Then it was supposedly fixed in

I've fixed it by adding a Set method for setting the both the current value and the last published value

ipfs/kubo@86b68db#diff-bf185c51dbf7a34872e42464354e5d12R328

(which now becomes obsolete with #47).

Another bug appeared in ipfs/kubo#4758 (comment)

When opening a file its state is not initialized and hence left at stateFlushed (I think this should be an explicit initialization even if the value is left at iota).

This is where the conversation dissolves so I think that's what needs to be fixed.

The only way I can see us get through that test case for TestFlushing is if we treat both stateSynced and stateFlushed the exact same way.

We have now removed Sync from the file descriptor interface (2f52311) so we should remove stateSynced (and its related logic).

I.E. Get the node, parent and name from the File->inode and call parent.updateChildEntry with this child{name, node}, since that is the only way we can unblock the WaitPublish() to prevent a permanent hang.

Not sure I follow but I trust you on this one since I haven't looked that closely at those commits, put it in a PR and we can evaluate it further.

Please work on top of #45 which does an important refactoring of the update mechanism so you don't have merge conflicts later.

nitishm pushed a commit to nitishm/go-mfs that referenced this issue Dec 27, 2018
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

nitishm commented Dec 29, 2018

@schomatis BTW this will need a change in go-ipfs as well since the File.Open has changed. See the first two file changes in ipfs/kubo@41c554b.
Let me know if I need to open a parallel PR in go-ipfs as well.

@schomatis
Copy link
Contributor Author

Yes, but I've already broken the API so I'm fine waiting to see how many PRs we can land here in the following days and then open a single PR in go-ipfs to match all the changes.

nitishm pushed a commit to nitishm/go-mfs that referenced this issue Dec 30, 2018
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.
@schomatis
Copy link
Contributor Author

With #49 done there's only one missing.

@aschmahmann
Copy link
Contributor

This repository is no longer maintained and has been copied over to Boxo. In an effort to avoid noise and crippling in the Boxo repo from the weight of issues of the past, we are closing most issues and PRs in this repo. Please feel free to open a new issue in Boxo (and reference this issue) if resolving this issue is still critical for unblocking or improving your usecase.

You can learn more in the FAQs for the Boxo repo copying/consolidation effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/ready Ready to be worked
Projects
No open projects
Archived in project
Development

No branches or pull requests

3 participants