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

Efficient "repo stat" (DiskUsage) and "--size-only" flag #5010

Merged
merged 4 commits into from
Jul 16, 2018

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented May 9, 2018

This makes use of the PersistentDatastore DiskUsage method to obtain the Repo's storage usage (GetStorageUsage()).

Additionally, the --size-only flag has been added to the "ipfs repo stat" command. This avoids counting the number of objects in the repository and returns faster.

Overseeds #4550

I had to make some gx magic to fix dependencies and be able to use latest go-datastore. go-libp2p-kad-dht depends on it and afaik the changes there cannot be integrated still etc. I'm happy to leave this hanging until this is sorted out. Thus, tagging this as blocked.

@hsanjuan hsanjuan added the status/blocked Unable to be worked further until needs are met label May 9, 2018
@hsanjuan hsanjuan self-assigned this May 9, 2018
@hsanjuan hsanjuan requested a review from Kubuxu as a code owner May 9, 2018 19:49
@ghost ghost added the status/in-progress In progress label May 9, 2018
@hsanjuan hsanjuan force-pushed the feat/diskusage2 branch 2 times, most recently from 6a47abb to bcbc15a Compare May 9, 2018 23:23
@whyrusleeping
Copy link
Member

@hsanjuan the DHT fixes have been merged, you should be unblocked here

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Jun 4, 2018

@whyrusleeping is that the case now?

@whyrusleeping
Copy link
Member

@hsanjuan yeah, #5007 merged

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Jun 4, 2018

Now rebased on master. Thanks @Stebalien for bubbling up deps on some other commit.

@hsanjuan hsanjuan added need/review Needs a review and removed status/blocked Unable to be worked further until needs are met status/in-progress In progress labels Jun 4, 2018
return nil
}

fmt.Fprintf(wtr, "NumObjects:\t%d\n", stat.NumObjects)
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a breaking change but I doubt anyone is relying on this order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope not. I can turn it around though and leave NumObjects as first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just change it. It's marginally better to not change the "API".

}

// RepoSize returns a *Stat object with the RepoSize and StorageMax fields set.
func RepoSize(ctx context.Context, n *core.IpfsNode) (*Stat, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit funky... what about:

type Size struct {
	RepoSize   uint64
	StorageMax uint64
}

type Stat struct {
	StorageSize
	NumObjects  uint64
	RepoPath    string
	Version     string
}

And then have RepoSize return Size (and, really, these functions should return be returning these by value). This would also avoid sending zero values for the missing fields.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is better

@hsanjuan
Copy link
Contributor Author

@Stebalien I have addressed the comments. However, since the command output only supports a single type, SizeStat needs to eventually become Stat. The final result is 0-valued json fields when doing a request with --size-only. (this is ok for me). If we wanted to return json without those fields, we would need a second command (ipfs repo size).

Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeEncoder(func(req *cmds.Request, w io.Writer, v interface{}) error {
stat, ok := v.(*corerepo.Stat)
if !ok {
fmt.Println("adios")
Copy link
Member

Choose a reason for hiding this comment

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

Debug print statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@hsanjuan hsanjuan requested a review from Stebalien June 18, 2018 09:11
This makes use of the PersistentDatastore DiskUsage method to
obtain the Repo's storage usage (GetStorageUsage()).

Additionally, the --size-only flag has been added to the
"ipfs repo stat" command. This avoids counting the number of objects
in the repository and returns faster.

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@ghost ghost added the status/in-progress In progress label Jul 8, 2018
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Jul 8, 2018

@whyrusleeping can we merge this please?

@Stebalien
Copy link
Member

@hsanjuan once we do the release which should happen any day now.

@whyrusleeping whyrusleeping merged commit 07feeec into master Jul 16, 2018
@ghost ghost removed the status/in-progress In progress label Jul 16, 2018
@whyrusleeping whyrusleeping deleted the feat/diskusage2 branch July 16, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants