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

coreapi unixfs: remove Cat, use sessions #5574

Merged
merged 6 commits into from
Oct 22, 2018
Merged

coreapi unixfs: remove Cat, use sessions #5574

merged 6 commits into from
Oct 22, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Oct 9, 2018

No description provided.

@magik6k magik6k requested a review from Stebalien October 9, 2018 16:59
@magik6k magik6k requested a review from Kubuxu as a code owner October 9, 2018 16:59
@ghost ghost assigned magik6k Oct 9, 2018
@ghost ghost added the status/in-progress In progress label Oct 9, 2018
Kubuxu
Kubuxu reviewed Oct 9, 2018
View reviewed changes
core/corehttp/gateway_handler.go Outdated
@@ -270,7 +269,7 @@ func (i *gatewayHandler) getOrHeadHandler(ctx context.Context, w http.ResponseWr
} else {
name = getFilename(urlPath)
}
i.serveFile(w, r, name, modtime, dr)
i.serveFile(w, r, name, modtime, dr.(io.ReadSeeker))
Copy link
Member

Choose a reason for hiding this comment

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

I am cautious of forced casts.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 9, 2018

Is there a reason for not returning it with Seeker interface by default?

@magik6k magik6k force-pushed the feat/coreapi/remove-cat branch 2 times, most recently from b595b1c to 21b1717 Compare October 10, 2018 13:40
@magik6k magik6k added the topic/core-api Topic core-api label Oct 14, 2018
hannahhoward
hannahhoward approved these changes Oct 16, 2018
View reviewed changes
Copy link
Contributor

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM, one non-blocking comment listen.

At the same time, I do not feel I have enough context to evaluate this code completely, and I would defer to someone with more expertise

core/commands/cat.go
if err != nil {
return nil, 0, err
}
offset = 0

size := uint64(read.Size() - uint64(count))
fsize, err = file.Size()
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit pick but why are you recalculating fsize again here when you appear to have set w/ same value on line 147

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a getter under the hood - https://github.com/ipfs/go-unixfs/blob/master/io/pbdagreader.go#L163

@magik6k
Copy link
Member Author

magik6k commented Oct 16, 2018

@Stebalien do you think we should test this on a gateway first or can this just be merged?

@magik6k
coreapi unixfs: remove Cat, use sessions
34b1313 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi unixfs: fix Get seeking in gateway
7915d26 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi unixfs: Return seeker from get
faf5230 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi unixfs: use session for resolving too
a4cb060 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Stebalien
Stebalien reviewed Oct 22, 2018
View reviewed changes
core/coreapi/interface/unixfs.go

// Cat returns a reader for the file
// TODO: Remove in favour of Get (if we use Get on a file we still have reader directly, so..)
Cat(context.Context, Path) (Reader, error)
Copy link
Member

Choose a reason for hiding this comment

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

We can probably get away with this but we'll need to be smart about whether we optimize for bandwidth or latency. That is, when calling Cat, we clearly care about reading the file in-order (latency to next byte). When calling Get, we're indicating we just want the entire file (maximize throughput overall).

We can still do this even if we remove the Cat function. However, we'll probably need to add some kind of option (or some method on the UnixfsFile interface).

We can handle this later, it's just something to keep in mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I'd say this is easy enough to handle later

Stebalien
Stebalien reviewed Oct 22, 2018
View reviewed changes
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.

No real objections other than we need to add a bit of documentation.

core/coreapi/path.go Outdated
@@ -73,3 +74,8 @@ func (api *CoreAPI) ResolvePath(ctx context.Context, p coreiface.Path) (coreifac

return coreiface.NewResolvedPath(ipath, node, root, gopath.Join(rest...)), nil
}

func (api *CoreAPI) getSession(ctx context.Context) *CoreAPI {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some documentation here. Specifically, we need to at least explain that the returned API has a read-only DAG.

core/corehttp/gateway_handler.go Outdated
case coreiface.ErrIsDir:
dir = true
switch {
case err == nil:
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? If we're going to go with something like this, a simple if err == nil { ... } else { ... } would probably be cleaner.

core/corehttp/gateway_handler.go Outdated
case err == nil:
dir = dr.IsDirectory()
if !dir {
defer dr.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Actually... shouldn't we be closing this regardless? (I have no idea, this just looks weird).

Copy link
Member Author

Choose a reason for hiding this comment

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

So technically for go-ipfs-files.File, in it's current form, Close should be only called for non-directories: https://github.com/ipfs/go-ipfs-files/blob/master/file.go#L18-L20

I'm working on refactoring files, making close valid for all files is one of the goals, but I wouldn't want to have this PR blocked on that refactor

Copy link
Member

Choose a reason for hiding this comment

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

SGTM.

@magik6k
coreapi unixfs: docs for getSession, use api.dag
838aeb9 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
coreapi unixfs: gw handler cleanup
519c67a 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@Stebalien Stebalien merged commit c4974d2 into master Oct 22, 2018
@ghost ghost removed the status/in-progress In progress label Oct 22, 2018
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
@Stebalien
Merge pull request ipfs/kubo#5574 from ipfs/feat/coreapi/remove-cat
a1303a5 
coreapi unixfs: remove Cat, use sessions

This commit was moved from ipfs/kubo@c4974d2
@hacdias hacdias deleted the feat/coreapi/remove-cat branch May 9, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu left review comments

@Stebalien Stebalien Stebalien left review comments

@hannahhoward hannahhoward hannahhoward approved these changes

Assignees

@magik6k magik6k

Labels
topic/core-api Topic core-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@magik6k @Kubuxu @Stebalien @hannahhoward