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

FPA add file #155

Merged
merged 12 commits into from Mar 2, 2020
Merged

FPA add file #155

merged 12 commits into from Mar 2, 2020

Conversation

@asutula
Copy link
Member

asutula commented Feb 28, 2020

Adds ability to add data to FPA from a local file.
Covers everything from the server side to client, cli and tests

closes #154

asutula added 8 commits Feb 26, 2020
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>
@asutula asutula requested review from sanderpick and jsign Feb 28, 2020
@asutula asutula self-assigned this Feb 28, 2020
@asutula

This comment has been minimized.

Copy link
Member Author

asutula commented Feb 28, 2020

Some extra noise in here from renaming things from "store" and "put" to "add"

}

var fpaAddCmd = &cobra.Command{
Use: "add [cid|path]",

This comment has been minimized.

Copy link
@asutula

asutula Feb 28, 2020

Author Member

Decided to combine adding by cid and path into this single command. Also using args now for the cid/path instead of flags.

This comment has been minimized.

Copy link
@sanderpick

sanderpick Mar 2, 2020

Member

Would be nice to handle stdin at some point. I was thinking the same for textile.

@@ -42,7 +42,7 @@ func RenderTable(writer io.Writer, header []string, data [][]string) {
table.SetBorder(false)
table.SetTablePadding("\t")
table.SetNoWhiteSpace(true)
headersColors := make([]tablewriter.Colors, len(data[0]))
headersColors := make([]tablewriter.Colors, len(header))

This comment has been minimized.

Copy link
@asutula

asutula Feb 28, 2020

Author Member

General bug


func (i *Instance) AddCid(ctx context.Context, c cid.Cid) error {
ar := i.auditor.Start(ctx, i.info.ID.String())
defer ar.Close()

This comment has been minimized.

Copy link
@asutula

asutula Feb 28, 2020

Author Member

Bug fix, pretty sure this should be deferred.

This comment has been minimized.

Copy link
@jsign

jsign Mar 2, 2020

Contributor

Yeah, actually the auditor concept is still quite undefined... but since it has a Close() better to do this.

asutula added 2 commits Feb 28, 2020
test fix
Signed-off-by: Aaron Sutula <hi@asutula.com>
Signed-off-by: Aaron Sutula <hi@asutula.com>

go receiveFile(srv, writer)

cid, err := i.AddFile(srv.Context(), reader)

This comment has been minimized.

Copy link
@asutula

asutula Feb 28, 2020

Author Member

@sanderpick I ended up letting the call to AddFile block and spun the data processing out in a goroutine... much cleaner, thx for the suggestion.

This comment has been minimized.

Copy link
@sanderpick
if sendErr != nil {
if sendErr == io.EOF {
var noOp interface{}
return nil, stream.RecvMsg(noOp)

This comment has been minimized.

Copy link
@asutula

asutula Feb 28, 2020

Author Member

Oh, this is important. I was thrown off by this for 24 hrs. Turns out that if the server returns an error in the middle of client streaming requests, you get back error io.EOF. If you want to see the actual error from the server, you have to get it with stream.RecvMsg(). Will get this updated elsewhere in the code base.

This comment has been minimized.

Copy link
@asutula

This comment has been minimized.

Copy link
@sanderpick

sanderpick Mar 2, 2020

Member

Good catch!

@jsign
jsign approved these changes Mar 2, 2020
Copy link
Contributor

jsign left a comment

LGTM! I left some minor comments.
I'll leave the ground green for merging this, and then dependabot prs.

defer cancel()

if len(args) != 1 {
Fatal(errors.New("you must provide a valid cid or file path"))

This comment has been minimized.

Copy link
@jsign

jsign Mar 2, 2020

Contributor

Nit: text error in this msg is the same as in line 73. Maybe should be diferent? (say, delete the valid word in this msg).


func (i *Instance) AddCid(ctx context.Context, c cid.Cid) error {
ar := i.auditor.Start(ctx, i.info.ID.String())
defer ar.Close()

This comment has been minimized.

Copy link
@jsign

jsign Mar 2, 2020

Contributor

Yeah, actually the auditor concept is still quite undefined... but since it has a Close() better to do this.

@jsign
jsign approved these changes Mar 2, 2020
Signed-off-by: Aaron Sutula <hi@asutula.com>
@asutula asutula merged commit 21389d4 into master Mar 2, 2020
2 checks passed
2 checks passed
Test
Details
DCO DCO
Details
@asutula asutula deleted the asutula/fpa-data branch Mar 2, 2020
Copy link
Member

sanderpick left a comment

LGTM! Left a couple comments.

if sendErr != nil {
if sendErr == io.EOF {
var noOp interface{}
return nil, stream.RecvMsg(noOp)

This comment has been minimized.

Copy link
@sanderpick

sanderpick Mar 2, 2020

Member

Good catch!

}

var fpaAddCmd = &cobra.Command{
Use: "add [cid|path]",

This comment has been minimized.

Copy link
@sanderpick

sanderpick Mar 2, 2020

Member

Would be nice to handle stdin at some point. I was thinking the same for textile.

@@ -110,6 +131,15 @@ func (i *Instance) storeInFIL(ctx context.Context, c cid.Cid) (ColdInfo, error)
return ci, nil
}

func (i *Instance) addToHotLayer(ctx context.Context, reader io.Reader) (*cid.Cid, error) {
path, err := i.ipfs.Unixfs().Add(ctx, files.NewReaderFile(reader), options.Unixfs.Pin(false))

This comment has been minimized.

Copy link
@sanderpick

sanderpick Mar 2, 2020

Member

You guys might consider trying to combine the pinning and adding. I'm guessing the same pin will be added in a different step.


go receiveFile(srv, writer)

cid, err := i.AddFile(srv.Context(), reader)

This comment has been minimized.

Copy link
@sanderpick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.