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 a --with-local option to ipfs files stat #4638

Merged
merged 8 commits into from
Feb 8, 2018

Conversation

MichaelMure
Copy link
Contributor

Ok, new attempt after #4541 and #4596.

This PR add a --with-local option to ipfs files stat that compute and display how much of a dag is local.

I had a small problem with a the functions defined in env.go that are not reachable from a subdir of core/commands so I copied the file for now. I'm not sure how you want to solve that.

On a side note, I would be very pleased if any version can make it to the upcoming RC and version.

@MichaelMure
Copy link
Contributor Author

@whyrusleeping @Stebalien @kevina please have a look.

@kevina
Copy link
Contributor

kevina commented Feb 1, 2018

I would also like to have @keks look over this if he is available as you are switching over the to new commands library.

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

Left one comment.

In addition the file core/commands/files/env.go is a copy of core/commands/env.go please remove it.

CumulativeSize uint64
Blocks int
Type string
WithLocality bool
Copy link
Contributor

@kevina kevina Feb 1, 2018

Choose a reason for hiding this comment

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

Please add `json:",omitempty"` after WithLocality so that the API output does not change if the --with-local option is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone to weight here ? For me it makes more sense like this. There is no compatibility problem, it's just an extra field. The problem is If WithLocality is set to false, the field is not present in the api anwser so you need to test if the field is present and if it's set to true. Not the end of the world, but ...

Copy link
Member

Choose a reason for hiding this comment

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

@MichaelMure No, the same struct would be used on both sides. theres no need to check if its set, we just don't want the API to change where its not wanted. @kevina is right here.

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

@kevina
Copy link
Contributor

kevina commented Feb 1, 2018

I had a small problem with a the functions defined in env.go that are not reachable from a subdir of core/commands so I copied the file for now. I'm not sure how you want to solve that.

Part of the problem is the files.go is in its own directory but not a proper package. Since this is just a single file, the easiest solution is to just move it. Commit b573768 (branch kevina/statlocal-fix1) does this. Feel free to cherry pick that change.

The other solution is to properly make files.go into its own package. I tried that in 554752c (branch kevina/statlocal-fix2) but ran into a import cycle:

import cycle not allowed
package github.com/ipfs/go-ipfs/cmd/ipfs
        imports github.com/ipfs/go-ipfs/core/commands
        imports github.com/ipfs/go-ipfs/core/commands/files
        imports github.com/ipfs/go-ipfs/core/commands
cmd/ipfs/Rules.mk:22: recipe for target 'cmd/ipfs/ipfs' failed

With a little more effort this could be properly fixed, but I think its best just to move files.go out of it own directory.

"rm": FilesRmCmd,
"flush": FilesFlushCmd,
"chcid": FilesChcidCmd,
"read": lgc.NewCommand(filesReadCmd),
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, You could just use the OldSubcommands map instead of calling legacy.NewCommand on all of these. I'm not that picky though.

switch fsn.Type() {
case mfs.TDir:
switch d.GetType() {
case ft.TDirectory, ft.THAMTShard:
Copy link
Member

Choose a reason for hiding this comment

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

why make this change?

Copy link
Member

Choose a reason for hiding this comment

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

d.GetType() can be of these types so if HAMTSharding was enabled we could potentially get an unrecognized node type error here, ya?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whyrusleeping because we want (or at least I do) support /ipfs/Qmwhatever. getNodeFromPath will return an ipld.Node instead of a mfs.FSNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frrist indeed. I got that from https://github.com/ipfs/go-ipfs/blob/master/mfs/system.go#L92 where there is the same problem. Would that be a file, directory or else ?

Copy link
Member

Choose a reason for hiding this comment

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

I think they must either be a file or a directory,

Files is an API for manipulating IPFS objects as if they were a unix filesystem. - file.go

The current definition looks good to me, what do ya'll think?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, MFS would handle /ipfs/ as a "mounted" subdirectory but we can fix that later.

@whyrusleeping
Copy link
Member

I would really prefer that we not move files.go back up into the "all the commands" directory. But it does make it pretty obnoxious to keep it down there...

@kevina
Copy link
Contributor

kevina commented Feb 2, 2018

@whyrusleeping

I would really prefer that we not move files.go back up into the "all the commands" directory. But it does make it pretty obnoxious to keep it down there...

To fix the import cycle we will need to move env.go into its own package also. I updated the kevina/statlocal-fix2 branch to do this in ae7324a.

@keks
Copy link
Contributor

keks commented Feb 2, 2018

FWIW I never understood why files, object etc. had their own subfolders. Why shouldn't they be in the main command folder?

@whyrusleeping
Copy link
Member

Yeah, it was really just because I wanted to try and break things up into smaller bits. I'm okay with having them all in the one folder now.

@kevina
Copy link
Contributor

kevina commented Feb 2, 2018

I'm okay either way. One reason files.go object.go and dht.go are in there own package is because they have there own prefix in the logger. When I moved files.go from its own package I needed to rename log to flog.

The disadvantage of having them in there own package is that common code will also need to be in its own package as I did in ae7324a, in case you missed it.

}
withLocal, _ := req.Options["with-local"].(bool)
if !withLocal {
cmds.EmitOnce(res, o)
Copy link
Member

Choose a reason for hiding this comment

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

you should just be able to use res.Emit(o)

cc @keks

@whyrusleeping
Copy link
Member

Alright, this LGTM. Will need a rebase.

Can I get a +1 from @Stebalien and @kevina ?

@kevina
Copy link
Contributor

kevina commented Feb 4, 2018

@whyrusleeping @MichaelMure because of the file move you need to be careful when reserving the conflict. Now that I worked out how to fix files.go so it can be in its own package, that might be best.

Note that I can take over the p.r. if it would help.

MichaelMure and others added 7 commits February 4, 2018 01:51
License: MIT
Signed-off-by: Michael Muré <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
It is a single file so putting it in its own package is a bit of an
overkill.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
License: MIT
Signed-off-by: Michael Muré <[email protected]>
@MichaelMure
Copy link
Contributor Author

@whyrusleeping rebase is done.
@kevina please take over if you want to make further changes. I can't push to ipfs/go-ipfs so I added you as a collaborator on my own repo if you want to stay in the same place.

@kevina
Copy link
Contributor

kevina commented Feb 4, 2018

I double checked the rebase and it looks good, the conflict was due to a dependency update which I made sure was correct.

@MichaelMure you should not need to give me access as I can push directly to your branch since you made a p.r. from it.

@whyrusleeping as I said before now that I think about it I am not sure moving files.go out of its directory is the best way and I have figured out to resolve the dependency cycle in ae7324a. object.go and dag.go are also in there own directory. What ever we do we should make things uniform.

[Truth be told I am okay either way, but it seams my comments are being lost in the noise and I am not sure anyone saw by commit that fixed the cycle. I apologize if I making things more complicated than need be.]

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Alright, this LGTM.

@MichaelMure You've tested this with your application right? I wouldnt want to merge this is only to not have it work out right for you.

@MichaelMure
Copy link
Contributor Author

@whyrusleeping it does works. Good thing is that I didn't even need to change js-ipfs-api.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. Not so happy with the naming (--with-local, WithLocality, etc.) but I can't come up with better names so... :shipit:.

switch fsn.Type() {
case mfs.TDir:
switch d.GetType() {
case ft.TDirectory, ft.THAMTShard:
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, MFS would handle /ipfs/ as a "mounted" subdirectory but we can fix that later.

@whyrusleeping whyrusleeping merged commit 25eeb1e into ipfs:master Feb 8, 2018
@whyrusleeping
Copy link
Member

Thanks for putting up with our indecision @MichaelMure :)

@MichaelMure
Copy link
Contributor Author

freddy

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.

6 participants