-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Rewrite pinning to store pins in IPFS objects #1225
Conversation
Travis failures look like OOM and timeouts. @chriscool When you say "all", that makes me think "out of scope for this issue" ;) @whyrusleeping @jbenet Let me know if there's anything unclear here, or something I can help with. I'm basically waiting to hear back from you two. |
PinWithMode(util.Key, PinMode) | ||
// RemovePinWithMode is for manually editing the pin structure. | ||
// Use with care! If used improperly, garbage collection may not | ||
// be successful. | ||
RemovePinWithMode(util.Key, PinMode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, youre probably right about moving these together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd think the other way was safer for users, but shrug
A few things that are confusing to me, but other than that, this looks really nice. Removes a bunch of things i wanted removed, and makes bunch of change towards a more sensible pinning setup |
@tv42 what I wanted to say is that as far as I know the other file comparison functions, like test_cmp, test_sort_cmp, are supposed to take the expected file first and the actual file second, and it would be nice if the comparison functions you introduce were following the same pattern too. |
@chriscool Hmm. Sharness's test_cmp is documented to take arguments in the order expected, actual. The preexisting code was |
@tv42 great! Thanks for fixing it! |
} | ||
rootKey := util.Key(rootKeyI.([]byte)) | ||
|
||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be wired up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that changes the Pinner API -> later.
@tv42 minor comments above o/ -- aside, this LGTM. |
Debian Stretch inside virtualbox, 2 GB RAM, 2 CPU cores allocated for VM (Core 2 Quad 9550). HW config 1VM is located on HDD. Guest root partition and guest /srv partition are located on separate physical HDDs (both SATA2). Test files located at /srv/test1, IPFS_PATH points to /srv/ipfs_conf.
sha256sum speed: 102.6 MB/s Adding urandom junk to ipfs:
Around 03:27:39 progress bar finishes, but ipfs does something, no output to console window. Process completes around 03:29:13. Average speed: 9.1 MB/s, iotop log Adding file full of 0xFF:
Average speed: 74.6 MB/s, iotop log |
HW config 2 (host OS)Core 2 Q9550, 4 CPU cores, 8 GB RAM, test file and ipfs_path on the same SSD.
Progress bar finishes at 10:55:15, process ends around 10:58, iotop log, average speed: 6.43 MB/s HW config 3 (host OS)IPFS_PATH on (fragmented?) HDD, test file on SSD:
I would discard this test. |
@vitzli nice! so this appears to be a little bit faster than master, right? |
@whyrusleeping I'm sure now that I've made a mistake, I commented on it in #1216, results are somewhat disappointing, pin branch seems to be slower than master, but I need somebody to verify my results.
Average time: 3m36.6s, 9.2 MB/s
Average time 3m56.7s, 8.5 MB/s |
Round 1 of the pinning rework was explicitly done so it doesn't change the APIs much. Any other way would have been too much chaos in the codebase. |
So further revisions wont require any sort of a migration, correct? |
It's always possible we discover the format was bad. I'd be surprised if that happened soon. |
Uhh, I added more tests and may or may not have found a bug. Please don't merge while I diagnose. Preparing migration code should still be safe. |
Pinner had method GetManual that returned a ManualPinner, so every Pinner had to implement ManualPinner anyway.
Platform-dependent behavior is not nice, and negative refcounts are not very useful.
WARNING: No migration performed! That needs to come in a separate commit, perhaps amended into this one. This is the minimal rewrite, only changing the storage from JSON(+extra keys) in Datastore to IPFS objects. All of the pinning state is still loaded in memory, and written from scratch on Flush. To do more would require API changes, e.g. adding error returns. Set/Multiset is not cleanly separated into a library, yet, as it's API is expected to change radically.
I saw it too, now. Looking.. |
@jbenet @whyrusleeping That's a dagmodifier error branch that explicitly unpins an object. // We found the correct child to write into
if cur+bs > offset {
// Unpin block
ckey := key.Key(node.Links[i].Hash)
dm.mp.RemovePinWithMode(ckey, pin.Indirect)
child, err := node.Links[i].GetNode(dm.ctx, dm.dagserv)
if err != nil {
// THIS TRIGGERS
return "", false, err
}
k, sdone, err := dm.modifyDag(child, offset-cur, data)
if err != nil {
return "", false, err
}
//NEVER GETS HERE
// pin the new node
dm.mp.PinWithMode(k, pin.Indirect) |
Well, the error at that point is a missing block too -- but that's the cause of the other missing block, that's what the test really craps out on. Lovely. |
Found it. |
Should be good again. @whyrusleeping FYI the dagmodifier changes. |
@tv42 good catch, thanks! |
merged into dev0.4.0 |
Can this be closed now? |
I think so yep :) 👍 thanks @tv42 |
Still needs a migration. Notes for that:
There's two parts:
The keys for that looked like /local/pins/{direct,recursive,indirect}/keys/* and can be just removed.
The keys for that look like /local/pins/{direct,recursive,indirect}/keys (parents of the above), contain json, and can be fed to the new pinning mechanism to generate the new state.
The direct and recursive json are arrays, the indirect json is a dict {key: refcount}.