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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jul 16, 2018
Merged

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

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 mentioned this pull request May 9, 2018
Closed
@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
Stebalien
Stebalien reviewed Jun 7, 2018
View reviewed changes
core/commands/repo.go Outdated
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".

core/corerepo/stat.go Outdated
}

// 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

@ghost ghost added the status/in-progress In progress label Jun 14, 2018
@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).

Stebalien
Stebalien reviewed Jun 14, 2018
View reviewed changes
core/commands/repo.go Outdated
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
@Stebalien Stebalien added RFM and removed status/in-progress In progress need/review Needs a review labels Jun 19, 2018
@hsanjuan hsanjuan mentioned this pull request Jun 20, 2018
Closed
@hsanjuan
Feat: use datastore.DiskUsage() and add --size-only to "repo stat"
79b388c 
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 <hector@protocol.ai>
@hsanjuan
Sharness: add tests for ipfs repo stat --size-only
37d4989 
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan
RepoStat: address review comments
9d88d2c 
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan
remove debug print statement
9c8d994 
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@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
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@hsanjuan hsanjuan

Labels
RFM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hsanjuan @whyrusleeping @Stebalien