Skip to content

Refactor UploadHandler and add ResumeUpload #166

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

Closed
wants to merge 21 commits into from
Closed

Refactor UploadHandler and add ResumeUpload #166

wants to merge 21 commits into from

Conversation

cnlangzi
Copy link
Contributor

@cnlangzi cnlangzi commented May 27, 2021

Checklist:

  • Refactor StorageHandler.WriteFile with Factory method pattern. Moved code to InsertFileCommand, UpdateFileCommand and DeleteFileCommand for flexible style
  • Added PATCH handler as ResumeUpload request
  • Added ResumeFileCommand to process ResumeUpload request
  • Added ResumeFileChange for file transaction
  • Added StreamMerkleHasher as stateful merkle tree to compute streaming file
  • Added WriteChunk in FileFSStore to flush chunk to temp file.

Refs:

  • uml flow

@cnlangzi cnlangzi changed the title feat(upload):refactor code for file upload/update/delete, and add res… [WIP] Refactor UploadHandler and add ResumeUpload May 27, 2021
@cnlangzi cnlangzi changed the title [WIP] Refactor UploadHandler and add ResumeUpload Refactor UploadHandler and add ResumeUpload Jun 11, 2021
@cnlangzi
Copy link
Contributor Author

./dev.local/cli.sh is linked to #145

@cnlangzi cnlangzi requested a review from Sriep June 11, 2021 15:44
@cnlangzi
Copy link
Contributor Author

@Sriep please help me to review PR if you get some time left , thanks.

Copy link
Contributor

@Sriep Sriep left a comment

Choose a reason for hiding this comment

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

Newly written functions need unit tests.

@@ -0,0 +1,2 @@
blobber
Copy link
Contributor

@Sriep Sriep Jun 12, 2021

Choose a reason for hiding this comment

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

Please remove.
Any scripts for building blobber should not use go source directories for executables please put them somewhere else.
If its just your own private build for debugging, then you should keep them private and not push to the main repository.

@@ -31,6 +34,7 @@ const (

var OperationNotApplicable = common.NewError("operation_not_valid", "Not an applicable operation")

// AllocationChangeProcessor request transaction of file operation. it is president in postgres, and can be rebuilt for next http reqeust(eg CommitHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

155 characters is quite long, maybe you could split the line.

IsFinal bool `json:"is_final,omitempty"`
//client side: unmarshal them from 'updateMeta'/'uploadMeta'
ConnectionID string `json:"connection_id" validation:"required"`
//client side:
Copy link
Contributor

Choose a reason for hiding this comment

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

This style is confusing to read. I suggest grouping the members under one ///client side comment.

@@ -532,6 +534,8 @@ func (fsh *StorageHandler) CommitWrite(ctx context.Context, r *http.Request) (*C
if err != nil {
return nil, err
}

fmt.Println(rootRef.Hash + ":" + strconv.FormatInt(int64(writeMarker.Timestamp), 10))
Copy link
Contributor

@Sriep Sriep Jun 12, 2021

Choose a reason for hiding this comment

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

Why are you using fmt.Println rather than a logger? If it is just a debug line please remove it.

@cnlangzi cnlangzi mentioned this pull request Jun 12, 2021
@cnlangzi
Copy link
Contributor Author

ignore it first. thanks.

@cnlangzi cnlangzi closed this Jun 14, 2021
@cnlangzi
Copy link
Contributor Author

cnlangzi commented Sep 6, 2021

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.

2 participants