Skip to content

Added StreamUpload #59

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 14 commits into from
Closed

Added StreamUpload #59

wants to merge 14 commits into from

Conversation

cnlangzi
Copy link
Contributor

@cnlangzi cnlangzi commented May 27, 2021

link to 0chain/blobber#166

Added StreamUpload with more clean design with parts

  • load/save progress
  • split data with ErasureEncoder, encrypt it if need
  • processUpload
  • processWriteMarker
  • processCommit

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.

Do we want to be including white papers in the project, rather than a link to the whitepaper? Along with an explanation as to why it's relevant.

commit to that point and easily roll back if there is an interruption in
the upload.

In the code that I had sent previously, this would be at line 61 where
Copy link
Contributor

Choose a reason for hiding this comment

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

Give a link; Line 61 of what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an email from Tom's forwarded by Saswata. just attach it as a reference. So I don't make any change on it.

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.

The problem is that most readers will not understand this. I think you should either turn this into a *.md and reformat it yourself or create a docs/merkle/streaming-merkle-hasher.md where you introduce this file and docs/merkle/streaming-merkle-hasher.node.

BTW is this the reference to streaming-merkle-hasher.node, because if so its by no mean clear and you should replace In the code that I had sent previously, this would be at line 61 with something like (use 0chain repository)

In [streaming-merkle-hasher.node line 61](https://github.com/cnlangzi/gosdk/blob/featur/continuous_upload/docs/merkle/streaming-merkle-hasher.node#L61)

Alternately you could say in the .md file, that the line 61 mentioned at the end of the letter is here and then give the link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

streaming-merkle-hasher.txt is original content of Tom's email. I think It is unnecessary to update it. It has been implemented in stream_merkle_hasher.go. It is safe to remove them now.
And @guruhubb maybe better to explain it than me.

@@ -0,0 +1,122 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this streaming-merkle-hasher.node file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an example code in nodejs. Please check streaming-merkle-hasher.txt. Tom explains it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where exactly? I found a reference in stream_merkle_hasher.go, is this an algorithm you have converted from javascript to go?

Does this file exist in an external GitHub repository? If so, I think a link to the file in its home repository would provide better context.

Copy link
Contributor Author

@cnlangzi cnlangzi Jun 12, 2021

Choose a reason for hiding this comment

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

Yes. It is written in go with some improvement in stream_merkle_hasher.go

The nodejs is written by Tom. They are forwarded by @guruhubb. just add it here as a reference. It is safe to remove now. I will remove them.

@cnlangzi cnlangzi changed the title feat(util):added StreamingMerkleHasher [WIP]: Added StreamUpload Jun 2, 2021
@cnlangzi
Copy link
Contributor Author

cnlangzi commented Jun 2, 2021

I am testing StreamUpload. so some code maybe be changed. please review it after I get test passed. thanks.

@cnlangzi cnlangzi changed the title [WIP]: Added StreamUpload Added StreamUpload Jun 11, 2021
@cnlangzi
Copy link
Contributor Author

FYI: devserver is linked to 0chain/blobber#145 . It is basic feature now. full feature will be followed in 0chain/blobber#145.

@cnlangzi cnlangzi requested a review from Sriep June 11, 2021 15:42
@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.

For unit tests you can check out 0chain/0chain#325. Also recent unit tests in zboxcore/sdk might help https://github.com/0chain/gosdk/blob/master/zboxcore/sdk/filestatsworker_test.go

FileStore <- FileSystem: FileRef with merkle leave hash
StorageHandler <- FileStore: FileOutputData
StorageHandler -> AllocationChangeProcessor: Add `InitFileChange`
UploadHandler <- StorageHandler: UploadResult
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this all happening in gosdk? If this for example some of these participants are in the blobber maybe you could indicate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is done in zbox and blobber with some update. I will update this UML. do you suggest where it should be put ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wherever you are using it. Is this the same file you put in the blobber PR description?

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. the UML diagram is built based on upload.puml. do you know https://plantuml.com/?


}

func (sb *StreamUploadBobbler) processUpload(su *StreamUpload, chunkIndex int, fileBytes, thumbnailBytes []byte, isFinal bool, wg *sync.WaitGroup) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can we have unit tests for new functions.

type StreamUploadOption func(su *StreamUpload)

// WithThumbnail add thumbnail. stream mode is unnecessary for thumbnail
func WithThumbnail(buf []byte) StreamUploadOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can we have unit tests for new functions.

@@ -354,6 +354,25 @@ func NewListRequest(baseUrl, allocation string, path string, auth_token string)
return req, nil
}

// NewUploadRequestWithMethod create a http reqeust of upload
func NewUploadRequestWithMethod(baseURL, allocation string, body io.Reader, method string) (*http.Request, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a unit test to confirm NewUploadRequestWithMethod returns the right http.Request. I know no one else has done it, but that's now reason why this PR should make the same mistake.

@cnlangzi
Copy link
Contributor Author

cnlangzi commented Jun 11, 2021

For unit tests you can check out 0chain/0chain#325. Also recent unit tests in zboxcore/sdk might help https://github.com/0chain/gosdk/blob/master/zboxcore/sdk/filestatsworker_test.go

@Sriep devserver is full feature blockchain mock for local development(it is not for unit tests). I have got them dropped from this PR. let us move and follow it on 0chain/blobber#145 thanks.


2) Otherwise:
2.a) set H to hash(HashNodes[index], H)
2.b) delete HashNodes[index] (i.e. set this position to empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comma, i.e. should be i.e.,

@@ -0,0 +1,122 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

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

Where exactly? I found a reference in stream_merkle_hasher.go, is this an algorithm you have converted from javascript to go?

Does this file exist in an external GitHub repository? If so, I think a link to the file in its home repository would provide better context.

commit to that point and easily roll back if there is an interruption in
the upload.

In the code that I had sent previously, this would be at line 61 where
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.

The problem is that most readers will not understand this. I think you should either turn this into a *.md and reformat it yourself or create a docs/merkle/streaming-merkle-hasher.md where you introduce this file and docs/merkle/streaming-merkle-hasher.node.

BTW is this the reference to streaming-merkle-hasher.node, because if so its by no mean clear and you should replace In the code that I had sent previously, this would be at line 61 with something like (use 0chain repository)

In [streaming-merkle-hasher.node line 61](https://github.com/cnlangzi/gosdk/blob/featur/continuous_upload/docs/merkle/streaming-merkle-hasher.node#L61)

Alternately you could say in the .md file, that the line 61 mentioned at the end of the letter is here and then give the link.

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