Skip to content
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

an attempt at making the editor more efficient #1567

Merged
merged 3 commits into from
Sep 5, 2015
Merged

Conversation

whyrusleeping
Copy link
Member

Using this, we would be able to pass an 'offline' dagstore to the editor, and then use the WriteOutputTo method on the editor to write the changed nodes to our actual dagservice.

@jbenet thoughts?

License: MIT
Signed-off-by: Jeromy [email protected]

@jbenet jbenet added the status/in-progress In progress label Aug 12, 2015
@jbenet
Copy link
Member

jbenet commented Aug 13, 2015

@whyrusleeping SGTM. the right solution, i think

@whyrusleeping
Copy link
Member Author

@whyrusleeping SGTM. the right solution, i think

Cool, I'll finish working it into the add command if you'd like.


func (e *Editor) WriteOutputTo(ds dag.DAGService) error {
return copyDag(e.GetNode(), e.ds, ds)
}
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to get called? shouldn't it be called by the add command at the end?

Copy link
Member

Choose a reason for hiding this comment

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

it's called now.

@whyrusleeping whyrusleeping force-pushed the editor-staging branch 2 times, most recently from 3333b81 to 48b115f Compare August 17, 2015 17:35
@whyrusleeping
Copy link
Member Author

I broke some test here... I'll need to take a look.

@whyrusleeping
Copy link
Member Author

@jbenet if you get a chance, could you take a look at the test failure here in t0080-repo.sh? Something is a little off in the pinning, and i'm not sure what...

@jbenet
Copy link
Member

jbenet commented Aug 19, 2015

Spelunking i also found this:

> cd test/sharness
> make bin/ipfs
> ./t0080-repo.sh
...
> cd trash\ directory.t0080-repo.sh
> ../bin/ipfs add -q multiblock
QmaPMMQBVm4mir7ZuL8b4MEFpEhoH2HA4oFdFAgSvsGCkG
Error: blockstore: block not found

the Error: blockstore: block not found shouldn't happen...

@@ -153,3 +148,32 @@ func rmLink(ctx context.Context, ds dag.DAGService, root *dag.Node, path []strin

return root, nil
}

func (e *Editor) WriteOutputTo(ds dag.DAGService) error {
return copyDag(e.GetNode(), e.ds, ds)
Copy link
Member

Choose a reason for hiding this comment

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

i think this needs what i wrote in 978c9fa#diff-bd0ca9f1e2d5c7ba128af01ec4c1131cR314

@jbenet
Copy link
Member

jbenet commented Aug 19, 2015

In my run (the hashes are different because we use random without a seed), i get:

> diff -u refsout_sorted indirectpins_sorted
--- refsout_sorted  2015-08-19 00:41:36.000000000 +0000
+++ indirectpins_sorted 2015-08-19 00:41:36.000000000 +0000
@@ -8,5 +8,6 @@
 QmY8EMo9ykRSkJAc9QTZbLUfusViyonfcbCb5PyKjYsQn2 indirect
 QmYCvbfNbCwFR45HiNP45rwJgvatpiW38D961L5qAhUM5Y indirect
 QmZTR5bcpQD7cFgTorqxZDYaew1Wqgfbd2ud9QqGPAkK2V indirect
+QmaPMMQBVm4mir7ZuL8b4MEFpEhoH2HA4oFdFAgSvsGCkG indirect
 Qmc8afjX8uzstEbK4ziHcW52TSHuL8UjSFp2NnAFF84DSR indirect
 QmdQs4tAY9Me7iUEv8n4ZD7jwEJQ4M8YCHRePRuh7CmBqr indirect

not ok 25 - 'ipfs pin ls --type=indirect' is correct

and QmaPMMQBVm4mir7ZuL8b4MEFpEhoH2HA4oFdFAgSvsGCkG is the root hash of running

> ipfs add -q multiblock
QmaPMMQBVm4mir7ZuL8b4MEFpEhoH2HA4oFdFAgSvsGCkG
Error: blockstore: block not found (<--- yeah, this shouldnt be there)

QmaPMMQBVm4mir7ZuL8b4MEFpEhoH2HA4oFdFAgSvsGCkG is listed as indirect. It should be recursive, because we're not wrapping.

This may be related to something i ran into when i wrote 978c9fa:

@jbenet jbenet mentioned this pull request Aug 22, 2015
16 tasks
@jbenet
Copy link
Member

jbenet commented Aug 23, 2015

@whyrusleeping I think I CRed this already. see #1567 (comment) -- otherwise LGTM

@whyrusleeping
Copy link
Member Author

@jbenet fixed this! should be good to go.

@jbenet
Copy link
Member

jbenet commented Sep 5, 2015

@whyrusleeping: 991cd4c committed sadhack.

ipfs add -p

.... err, i meant

git add -p

@whyrusleeping
Copy link
Member Author

@jbenet nuuuuu. the more i have to do this, the more resolve i get to fix the problem in the first place

@jbenet
Copy link
Member

jbenet commented Sep 5, 2015

Rebased on top of #1655

jbenet added a commit that referenced this pull request Sep 5, 2015
an attempt at making the editor more efficient
@jbenet jbenet merged commit 637e221 into master Sep 5, 2015
@jbenet jbenet removed the status/in-progress In progress label Sep 5, 2015
@jbenet jbenet deleted the editor-staging branch September 5, 2015 03:03
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants