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

add ipfs dag stat command #7553

Merged
merged 5 commits into from
Aug 17, 2020
Merged

add ipfs dag stat command #7553

merged 5 commits into from
Aug 17, 2020

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.

core/commands/dag/dag.go Outdated Show resolved Hide resolved
core/commands/dag/dag.go Show resolved Hide resolved
@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?

core/commands/dag/dag.go Outdated Show resolved Hide resolved
@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.

@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