-
-
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
dag: Support multiple files in put #4254
Conversation
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
test_expect_success "dag put multiple files" ' | ||
printf {\"foo\":\"bar\"} > a.json && | ||
printf {\"foo\":\"baz\"} > b.json && | ||
ipfs dag put a.json b.json > dag_put_out |
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.
Awesome!
Will test this week how many files it can get at once w/o choking. Thanks a lot for adding this!
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.
Nice! Just need to fix the pin lock.
core/commands/dag/dag.go
Outdated
if !ok { | ||
return nil, fmt.Errorf("expected a different object in marshaler") | ||
fmt.Println(reflect.TypeOf(res.Output())) |
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.
Leftovers from debugging.
core/commands/dag/dag.go
Outdated
@@ -100,52 +97,86 @@ into an object of the specified format. | |||
defer n.Blockstore.PinLock().Unlock() |
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.
Given that you add the blocks/pin in a go routine, this needs to go somewhere else (i.e., in the go routine). Also, if you want to improve this slightly, it's probably best not to hold this lock while waiting on the client (either receiving or sending data).
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
629cef5
to
03cf65b
Compare
Fixed |
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.
LGTM
core/commands/dag/dag.go
Outdated
defer n.Blockstore.PinLock().Unlock() | ||
} | ||
|
||
b := n.DAG.Batch() |
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.
We should put all dag nodes being added into the same batch here, i would move this batch creation up above the start of the for loop.
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.
Are you sure it is right thing to do? What if I start adding 100GB of DAGs?
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.
That was my thinking there, though we might just say that users should care about their batch sizes.
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.
The DAG batch is size limited, it will autoflush if you put too many items into it
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.
done
core/commands/dag/dag.go
Outdated
|
||
cid = nds[0].Cid() | ||
if dopin { | ||
n.Pinning.PinWithMode(cid, pin.Recursive) |
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.
Also, since we're batching, we should just gather up all the things we're going to pin into an array, pin them all at once at the end, then only call Pinning.Flush once
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
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.
👍
See #4234