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

Support non-seekable stdin (- arg) in "fs upload" command #1672

Merged
merged 4 commits into from
Mar 25, 2021
Merged

Support non-seekable stdin (- arg) in "fs upload" command #1672

merged 4 commits into from
Mar 25, 2021

Conversation

arielshaqed
Copy link
Contributor

Parity with aws s3 cp - ... on this.

@arielshaqed arielshaqed force-pushed the feature/client-allow-nonseekable-upload branch from 5633607 to 76c9589 Compare March 24, 2021 09:43
@arielshaqed
Copy link
Contributor Author

Oh yeah, fs upload now checks for errors, which is kinda a big deal when storing a file.

@arielshaqed arielshaqed linked an issue Mar 24, 2021 that may be closed by this pull request
lakectl fs upload does not check for errors on Close #1673
Closed
@codecov-io
Copy link

codecov-io commented Mar 24, 2021 •
edited
Loading

Codecov Report

Merging #1672 (28a69ae) into master (39323ff) will decrease coverage by 0.48%.
The diff coverage is 2.45%.

❗ Current head 28a69ae differs from pull request most recent head f1997e1. Consider uploading reports for the commit f1997e1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1672      +/-   ##
==========================================
- Coverage   39.40%   38.92%   -0.49%     
==========================================
  Files         167      167              
  Lines       13611    13771     +160     
==========================================
- Hits         5364     5360       -4     
- Misses       7485     7645     +160     
- Partials      762      766       +4
Impacted Files Coverage Δ
pkg/api/client.go 3.82% <0.00%> (-0.50%) ⬇️
pkg/block/s3/adapter.go 0.00% <0.00%> (ø)
pkg/catalog/catalog.go 18.41% <0.00%> (-0.40%) ⬇️
pkg/graveler/graveler.go 34.95% <0.00%> (-0.18%) ⬇️
pkg/api/controller.go 32.99% <2.63%> (-1.23%) ⬇️
pkg/block/namespace.go 58.33% <100.00%> (ø)
pkg/graveler/hooks_handler.go 100.00% <100.00%> (ø)
pkg/graveler/committed/batch.go 94.44% <0.00%> (-5.56%) ⬇️
pkg/pyramid/tier_fs.go 56.76% <0.00%> (-1.75%) ⬇️

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 673c443...f1997e1. Read the comment docs.

@arielshaqed arielshaqed force-pushed the feature/client-allow-nonseekable-upload branch from bef56df to e0db727 Compare March 24, 2021 12:17
johnnyaug
johnnyaug approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@johnnyaug johnnyaug left a comment

Choose a reason for hiding this comment

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

LGTM + minor comments

cmd/lakectl/cmd/input.go Outdated
Comment on lines 64 to 71
func (d *deleteOnClose) Read(p []byte) (n int, err error) {
return d.File.Read(p)
}

func (d *deleteOnClose) Seek(offset int64, whence int) (int64, error) {
return d.File.Seek(offset, whence)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

cmd/lakectl/cmd/input.go Outdated

func (d *deleteOnClose) Close() error {
if err := os.Remove(d.Name()); err != nil {
d.File.Close() // Close failure is unimportant on read, but data definitely stays!
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't understand this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "safe if close fails".

arielshaqed
arielshaqed commented Mar 24, 2021
View reviewed changes
Copy link
Contributor Author

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

PTAL (also fixed go fmt).

cmd/lakectl/cmd/input.go Outdated
Comment on lines 64 to 71
func (d *deleteOnClose) Read(p []byte) (n int, err error) {
return d.File.Read(p)
}

func (d *deleteOnClose) Seek(offset int64, whence int) (int64, error) {
return d.File.Seek(offset, whence)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

cmd/lakectl/cmd/input.go Outdated

func (d *deleteOnClose) Close() error {
if err := os.Remove(d.Name()); err != nil {
d.File.Close() // Close failure is unimportant on read, but data definitely stays!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "safe if close fails".

@arielshaqed
Copy link
Contributor Author

Actually pulling once tests passed -- nothing major to cancel approval. Thanks!

@arielshaqed arielshaqed force-pushed the feature/data-upload-server branch from 95166e9 to d5628e2 Compare March 25, 2021 09:24
Base automatically changed from feature/data-upload-server to master March 25, 2021 15:08
@arielshaqed arielshaqed force-pushed the feature/client-allow-nonseekable-upload branch 2 times, most recently from f1997e1 to ba2a5b9 Compare March 25, 2021 15:38
@arielshaqed arielshaqed force-pushed the feature/client-allow-nonseekable-upload branch from ba2a5b9 to c3494f0 Compare March 25, 2021 15:40
@arielshaqed
Copy link
Contributor Author

Thanks!

Pulling once tests pass...

@arielshaqed arielshaqed merged commit f9bcb3d into master Mar 25, 2021
@arielshaqed arielshaqed deleted the feature/client-allow-nonseekable-upload branch March 25, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnyaug johnnyaug Awaiting requested review from johnnyaug

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lakectl fs upload does not check for errors on Close
3 participants
@arielshaqed @codecov-io @johnnyaug