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

feat: enable bytes read tracking #1074

Merged
merged 31 commits into from May 13, 2020
Merged

feat: enable bytes read tracking #1074

merged 31 commits into from May 13, 2020

Conversation

AVaksman
Copy link
Contributor

@AVaksman AVaksman commented Feb 13, 2020 •
edited

Design: Changes to file.createWriteStream()

  • Introduce a ProgressStream class that extends from native Trasform

    • on every chunk emits a 'progress' event with total calculated bytesRead value.

  • add an instance of ProgressStream to the pipe.

  • If options.onUploadProgress is provided by user add a 'progress' listener that passes the total
    bytesRead value to options.onUploadProgress.

  • Ensure the tests and linter pass

  • Code coverage does not decrease (if any source code was changed)

  • Appropriate docs were updated (if necessary)

Fixes #956

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2020
@AVaksman AVaksman changed the title Enable bytes read tracking feat: enable bytes read tracking Feb 13, 2020
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 13, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 13, 2020
stephenplusplus
stephenplusplus suggested changes Feb 13, 2020
View reviewed changes
src/file.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
system-test/storage.ts Outdated Show resolved Hide resolved
src/file.ts Outdated Show resolved Hide resolved
@stephenplusplus
Copy link
Contributor

@AVaksman I forgot about this: https://github.com/googleapis/gcs-resumable-upload/blob/4cec8f74a408ea8dc03e3395555189fa6b09add0/src/index.ts#L451

Looks like we could just forward that event to the user (during resumable uploads, anyway)

@AVaksman
Copy link
Contributor Author

@AVaksman I forgot about this: https://github.com/googleapis/gcs-resumable-upload/blob/4cec8f74a408ea8dc03e3395555189fa6b09add0/src/index.ts#L451

Looks like we could just forward that event to the user (during resumable uploads, anyway)

That's good!
Should we worry about the simple uploads as well (for consistency)?

@stephenplusplus
Copy link
Contributor

Should we worry about the simple uploads as well (for consistency)?

Yes, I suppose we should. What do you think?

@AVaksman AVaksman mentioned this pull request Mar 2, 2020
Merged
3 tasks
@AVaksman AVaksman added the status: blocked Resolving the issue is dependent on other work. label Mar 2, 2020
@AVaksman
Copy link
Contributor Author

AVaksman commented Mar 4, 2020 •
edited

Blocked by

stephenplusplus
stephenplusplus reviewed Mar 16, 2020
View reviewed changes
src/file.ts Outdated
@@ -1698,7 +1701,7 @@ class File extends ServiceObject<File> {
pumpify([
gzip ? zlib.createGzip() : through(),
validateStream,
fileWriteStream,
fileWriteStream.on('progress', evt => stream.emit('progress', evt)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this event handler assignment on its own line? Just makes it easier to see what the code does line-by-line.

stephenplusplus
stephenplusplus approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@stephenplusplus stephenplusplus left a comment

Choose a reason for hiding this comment

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

Just a subjective suggestion, but LGTM.

@AVaksman
Copy link
Contributor Author

AVaksman commented Apr 2, 2020 •
edited

@bcoe
Now that @google-common v3 is released and progress events from simple upload are available (#540) (1834059) all system tests are passing.
However this PR should not be merged until Node 8 is depricated.

@stephenplusplus stephenplusplus added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 2, 2020
@AVaksman AVaksman removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: blocked Resolving the issue is dependent on other work. labels Apr 20, 2020
@codecov
Copy link

codecov bot commented Apr 27, 2020 •
edited

Codecov Report

Merging #1074 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1074   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          12       12           
  Lines       11474    11474           
  Branches      597      597           
=======================================
  Hits        11378    11378           
  Misses         95       95           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 195cd46...195cd46. Read the comment docs.

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
bcoe
bcoe approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I think this feature is really slick, if @stephenplusplus has given his 👍 I say ship it.

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020
@AVaksman AVaksman requested a review from a team as a code owner May 7, 2020 03:56
@stephenplusplus stephenplusplus added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 12, 2020
@stephenplusplus stephenplusplus merged commit 0776a04 into googleapis:master May 13, 2020
@gajus
Copy link

gajus commented Jun 22, 2020

There does not seem to be a way to track progress using the upload method.

@gajus
Copy link

gajus commented Jun 22, 2020

I was wrong. onUploadProgress is supported, but not documented. Seems like a documentation bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe approved these changes

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

progress tracking when uploading big file
7 participants
@AVaksman @stephenplusplus @gajus @bcoe @googlebot @yoshi-kokoro @jkwlui