Skip to content

add ipfs dag stat command#7553

Merged
aschmahmann merged 5 commits intomasterfrom
feat/dag-size-cmd
Aug 17, 2020
Merged

add ipfs dag stat command#7553
aschmahmann merged 5 commits intomasterfrom
feat/dag-size-cmd

Conversation

@aschmahmann
Copy link
Contributor

Adds a command to get simple stats out from a DAG (e.g. how big is it and how many blocks does it have).

This PR certainly has room for improvement, but adding this command is probably better than not.

Comment on lines +713 to +715
if len(rp.Remainder()) > 0 {
return fmt.Errorf("cannot return size for anything other than a DAG with a root CID")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is supportable, but didn't seem necessary for an initial release of this command. Open to counter opinions.

@aschmahmann aschmahmann requested a review from petar July 20, 2020 14:11
@achingbrain
Copy link
Member

This is really useful and something I use ipfs files stat for all the time.

It would be great if this could be a top level command, e.g. ipfs stat /ipfs/path instead of tucking it away under the dag subcommand.

@aschmahmann
Copy link
Contributor Author

It would be great if this could be a top level command, e.g. ipfs stat /ipfs/path instead of tucking it away under the dag subcommand.

We have ipfs x stat in a few different places already (ipfs bitswap stat, ipfs files stat) I think making it top level would be a little more confusing. It also makes it a little more clear what the difference is between ipfs files stat and ipfs dag stat one of them operates on UnixFS objects so could return things like "the file size", the other operates on DAGs and so returns IPLD block information.

@aschmahmann
Copy link
Contributor Author

@achingbrain for reasons... we also have ipfs stats X. It seems like moving data based stats (e.g. files, object, dag) as aliases under the ipfs stats header was examined at #2810 (comment) and passed on.

I don't really have strong feelings about these aliases (other then I don't know why we have them at all), so if you'd like ipfs stats dag = ipfs dag stat we could do that in another PR where we move the other data based commands. WDYT?

@aschmahmann aschmahmann force-pushed the feat/dag-size-cmd branch 2 times, most recently from e7b35d2 to 3a09089 Compare August 17, 2020 10:21
@aschmahmann aschmahmann marked this pull request as ready for review August 17, 2020 10:31
Comment on lines +769 to +781
dagStats = out
fmt.Fprintf(os.Stderr, "%v\r", out)
}
return re.Emit(dagStats)
},
},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, event *DagStat) error {
_, err := fmt.Fprintf(
w,
"%v\n",
event,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like the pattern we use for emitting updates as well as a final result, but would like some confirmation that this is reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

It is... I just wish we had a better way.

Comment on lines +271 to +272

test_expect_success "dag stat of simple IPLD object" '
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a few tests here, do we need more before shipping this?

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable.


dagstats := &DagStat{}
err = traverse.Traverse(obj, traverse.Options{
DAG: api.Dag(),
Copy link
Member

Choose a reason for hiding this comment

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

We should probably try to create a bitswap session here, but I'm fine punting that if necessary (this API will usually be called on local DAGs, I assume).

Comment on lines +769 to +781
dagStats = out
fmt.Fprintf(os.Stderr, "%v\r", out)
}
return re.Emit(dagStats)
},
},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, event *DagStat) error {
_, err := fmt.Fprintf(
w,
"%v\n",
event,
)
Copy link
Member

Choose a reason for hiding this comment

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

It is... I just wish we had a better way.

Comment on lines +271 to +272

test_expect_success "dag stat of simple IPLD object" '
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable.

@aschmahmann aschmahmann requested a review from Stebalien August 17, 2020 18:17
@hacdias hacdias deleted the feat/dag-size-cmd branch May 9, 2023 11:00
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.

5 participants