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

remove the fullSync option from updateChildEntry #45

Merged
merged 2 commits into from
Dec 27, 2018
Merged

remove the fullSync option from updateChildEntry #45

merged 2 commits into from
Dec 27, 2018

Conversation

schomatis
Copy link
Contributor

Closes #39.

@schomatis
remove the fullSync option from updateChildEntry
1bbc52d 
Make `updateChildEntry` always propagate the update all the way up to the root,
the equivalent of calling it always with `fullSync` set.

The case of calling it without setting `fullSync` (a kind of "half-update")
where only the parent's directory UnixFS node was updated (but nothing else,
leaving the root outdated) seemed of little used. This helps to simplify the
logic around the update mechanism in MFS.
@schomatis schomatis added the status/in-progress In progress label Dec 21, 2018
@schomatis schomatis added this to the Review the MFS interface milestone Dec 21, 2018
@schomatis schomatis self-assigned this Dec 21, 2018
@schomatis schomatis force-pushed the fix/flush/remove-fullsync branch from 1226f22 to 88b8e7b Compare December 21, 2018 20:38
@schomatis schomatis changed the title [WIP] remove the fullSync option from updateChildEntry remove the fullSync option from updateChildEntry Dec 21, 2018
@schomatis schomatis requested a review from Stebalien December 21, 2018 20:40
@schomatis schomatis added needs review and removed status/in-progress In progress labels Dec 21, 2018
@schomatis
Copy link
Contributor Author

/cc @Stebalien

@schomatis schomatis added the status/in-progress In progress label Dec 21, 2018
@schomatis
Copy link
Contributor Author

Test failing.

@schomatis
Copy link
Contributor Author

There is a race while encoding the ProtoNodes when flushing the MFS (adding it to the DAG service). May be related to ipfs/kubo#4784.

Note to self: this patch should have not increased the number of flushes (saves to DAG service), if anything it should have been reduced them (if sync is set in the file descriptor we repeat the operation we were doing without the patch and if it isn't we don't do anything), so why decreasing the number of operations created the race (if that is indeed related to the cause)? (Increasing the number of routines and iterations for TestMfsStress in master doesn't fail).

@schomatis
merge closeChildUpdate with flushCurrentNode
8d26210 
Without the `sync` logic (now removed) the code is simplified for these tightly
coupled methods (the first one was the only caller of the second) to be merged
in a single `localUpdate` method.
@schomatis schomatis force-pushed the fix/flush/remove-fullsync branch from 9afa20a to 8d26210 Compare December 22, 2018 01:05
@schomatis
Copy link
Contributor Author

I took the DAG.Add call outside the lock and then I wondered why I was getting a data race in the DAG service.. 😑

@schomatis schomatis removed the status/in-progress In progress label Dec 22, 2018
@ghost ghost added the status/in-progress In progress label Dec 22, 2018
@schomatis schomatis removed the status/in-progress In progress label Dec 22, 2018
@schomatis schomatis force-pushed the fix/flush/remove-fullsync branch from 9790794 to 6c6a2d1 Compare December 22, 2018 01:17
@ghost ghost added the status/in-progress In progress label Dec 22, 2018
@schomatis schomatis removed the status/in-progress In progress label Dec 22, 2018
@schomatis
Copy link
Contributor Author

(last update wafflebot, I swear)

@schomatis schomatis mentioned this pull request Dec 23, 2018
Closed
4 tasks
nitishm pushed a commit to nitishm/go-mfs that referenced this pull request Dec 25, 2018
fix/32/pr-ports : Pulled changes from PR 4517
3191540 
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`.
@schomatis schomatis mentioned this pull request Dec 26, 2018
Merged
Stebalien
Stebalien suggested changes Dec 27, 2018
View reviewed changes
dir.go Outdated Show resolved Hide resolved
root.go Show resolved Hide resolved
@schomatis schomatis force-pushed the fix/flush/remove-fullsync branch from 6c6a2d1 to 8d26210 Compare December 27, 2018 00:22
@ghost ghost added the status/in-progress In progress label Dec 27, 2018
@schomatis
Copy link
Contributor Author

@Stebalien Fixed, thanks for spotting the lock bug.

@Stebalien
Copy link
Member

LGTM: Merge when ready.

Not locking while adding to the local dagservice would be nice if you'd like to do that in a followup PR. We just need to break the first part of that function into a helper function (so we get a new scope for the defer statement).

@schomatis
Copy link
Contributor Author

We just need to break the first part of that function into a helper function (so we get a new scope for the defer statement).

Agreed.

@schomatis schomatis merged commit 4fb6dc4 into master Dec 27, 2018
@ghost ghost removed the status/in-progress In progress label Dec 27, 2018
@schomatis schomatis deleted the fix/flush/remove-fullsync branch December 27, 2018 00:36
@schomatis schomatis mentioned this pull request Dec 27, 2018
Closed
nitishm pushed a commit to nitishm/go-mfs that referenced this pull request Dec 30, 2018
fix/32/pr-ports : Pulled changes from PR 4517
2642dbf 
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`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Stebalien Stebalien Stebalien approved these changes

Assignees

@schomatis schomatis

Labels
None yet
Projects
None yet
Milestone
Review the MFS interface
Development

Successfully merging this pull request may close these issues.
2 participants
@schomatis @Stebalien