From 54fcd1f1b8b37e4abb6faddf220b7dc21740dee2 Mon Sep 17 00:00:00 2001 From: Steve McCanne Date: Fri, 1 Nov 2024 05:46:53 -0700 Subject: [PATCH] simplify file not found messages - less is more (#5414) This commit simplifies file-not-found message by dropping the extra output of the file:/// absolute paths. We also noticed that "super:" was prefixed some of the times to errors and not others, so we took the "super:" prefix out in the one place it was used. When a data lake object is not found, the full path is printed. This happens on the vaccum test. This is not the right UX when trying to scanned vaccumed parts of the lake, but it's how it works for now. --- cmd/super/root/command.go | 2 +- cmd/super/ztests/from-file-error.yaml | 2 +- cmd/super/ztests/from-pool-error.yaml | 2 +- cmd/super/ztests/no-files.yaml | 4 ++-- cmd/super/ztests/single-arg-error.yaml | 2 +- compiler/parser/ztests/syntax-error.yaml | 2 +- docs/tutorials/schools.md | 2 +- lake/data/reader.go | 3 ++- lake/ztests/vacuum.yaml | 4 ++-- pkg/storage/file.go | 25 ++++++++++++------------ pkg/storage/s3.go | 14 ++++++------- 11 files changed, 31 insertions(+), 31 deletions(-) diff --git a/cmd/super/root/command.go b/cmd/super/root/command.go index 1a526ae9a1..aa2c3fe134 100644 --- a/cmd/super/root/command.go +++ b/cmd/super/root/command.go @@ -146,7 +146,7 @@ func (c *Command) Run(args []string) error { } paths, ast, null, err := c.queryFlags.ParseSourcesAndInputs(c.query, args) if err != nil { - return fmt.Errorf("super: %w", err) + return err } zctx := super.NewContext() local := storage.NewLocalEngine() diff --git a/cmd/super/ztests/from-file-error.yaml b/cmd/super/ztests/from-file-error.yaml index 1bca163911..a4c526c014 100644 --- a/cmd/super/ztests/from-file-error.yaml +++ b/cmd/super/ztests/from-file-error.yaml @@ -9,4 +9,4 @@ inputs: outputs: - name: stderr regexp: | - a.jsup: file:///.*/a.jsup: file does not exist.* + a.jsup: file does not exist.* diff --git a/cmd/super/ztests/from-pool-error.yaml b/cmd/super/ztests/from-pool-error.yaml index 9e1cefa7db..fa2a25a898 100644 --- a/cmd/super/ztests/from-pool-error.yaml +++ b/cmd/super/ztests/from-pool-error.yaml @@ -4,4 +4,4 @@ script: | outputs: - name: stderr regexp: | - a: file:///.*/a: file does not exist + a: file does not exist diff --git a/cmd/super/ztests/no-files.yaml b/cmd/super/ztests/no-files.yaml index c71b4ac1ef..fb418a113c 100644 --- a/cmd/super/ztests/no-files.yaml +++ b/cmd/super/ztests/no-files.yaml @@ -1,9 +1,9 @@ script: | - ! super -z -c doesnotexist + ! super -z doesnotexist outputs: - name: stdout data: "" - name: stderr data: | - super: no data source found + doesnotexist: file does not exist diff --git a/cmd/super/ztests/single-arg-error.yaml b/cmd/super/ztests/single-arg-error.yaml index 3787b0f86e..e09f7bcb16 100644 --- a/cmd/super/ztests/single-arg-error.yaml +++ b/cmd/super/ztests/single-arg-error.yaml @@ -4,6 +4,6 @@ script: | outputs: - name: stderr data: | - super: parse error at line 1, column 26: + parse error at line 1, column 26: file sample.jsup | count( === ^ === diff --git a/compiler/parser/ztests/syntax-error.yaml b/compiler/parser/ztests/syntax-error.yaml index 091c594f8a..53b7af9ee8 100644 --- a/compiler/parser/ztests/syntax-error.yaml +++ b/compiler/parser/ztests/syntax-error.yaml @@ -8,6 +8,6 @@ inputs: outputs: - name: stderr data: | - super: parse error at line 1, column 12: + parse error at line 1, column 12: count() by ,x,y === ^ === diff --git a/docs/tutorials/schools.md b/docs/tutorials/schools.md index 890fd3aba8..60e6fc705e 100644 --- a/docs/tutorials/schools.md +++ b/docs/tutorials/schools.md @@ -235,7 +235,7 @@ super -z -c 'Defunct=' *.jsup ``` produces ```mdtest-output -super: parse error at line 1, column 9: +parse error at line 1, column 9: Defunct= === ^ === ``` diff --git a/lake/data/reader.go b/lake/data/reader.go index e48ad0f458..237dba67da 100644 --- a/lake/data/reader.go +++ b/lake/data/reader.go @@ -2,6 +2,7 @@ package data import ( "context" + "fmt" "io" "github.com/brimdata/super/lake/seekindex" @@ -22,7 +23,7 @@ func (o *Object) NewReader(ctx context.Context, engine storage.Engine, path *sto objectPath := o.SequenceURI(path) reader, err := engine.Get(ctx, objectPath) if err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", objectPath, err) } var r io.Reader var readBytes int64 diff --git a/lake/ztests/vacuum.yaml b/lake/ztests/vacuum.yaml index 180a57fab5..0a1dfd3fd4 100644 --- a/lake/ztests/vacuum.yaml +++ b/lake/ztests/vacuum.yaml @@ -16,5 +16,5 @@ outputs: would vacuum 1 object vacuumed 1 object - name: stderr - regexp: - file:.*file does not exist + regexp: | + file:///.*file does not exist diff --git a/pkg/storage/file.go b/pkg/storage/file.go index 8f54737332..eeb1aacbdc 100644 --- a/pkg/storage/file.go +++ b/pkg/storage/file.go @@ -3,7 +3,6 @@ package storage import ( "bytes" "context" - "fmt" "io" "io/fs" "os" @@ -31,26 +30,26 @@ func NewFileSystem() *FileSystem { func (f *FileSystem) Get(ctx context.Context, u *URI) (Reader, error) { r, err := pkgfs.Open(u.Filepath()) - return &fileSizer{r, u}, wrapfileError(u, err) + return &fileSizer{r, u}, fileErr(err) } func (f *FileSystem) Put(_ context.Context, u *URI) (io.WriteCloser, error) { path := u.Filepath() if err := f.checkPath(path); err != nil { - return nil, wrapfileError(u, err) + return nil, fileErr(err) } w, err := pkgfs.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, f.perm) - return w, wrapfileError(u, err) + return w, fileErr(err) } func (f *FileSystem) PutIfNotExists(_ context.Context, u *URI, b []byte) error { path := u.Filepath() if err := f.checkPath(path); err != nil { - return wrapfileError(u, err) + return fileErr(err) } file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_EXCL, f.perm) if err != nil { - return wrapfileError(u, err) + return fileErr(err) } _, err = io.Copy(file, bytes.NewReader(b)) if err != nil { @@ -62,7 +61,7 @@ func (f *FileSystem) PutIfNotExists(_ context.Context, u *URI, b []byte) error { } func (f *FileSystem) Delete(_ context.Context, u *URI) error { - return wrapfileError(u, os.Remove(u.Filepath())) + return fileErr(os.Remove(u.Filepath())) } func (f *FileSystem) DeleteByPrefix(_ context.Context, u *URI) error { @@ -72,7 +71,7 @@ func (f *FileSystem) DeleteByPrefix(_ context.Context, u *URI) error { func (f *FileSystem) Size(_ context.Context, u *URI) (int64, error) { info, err := os.Stat(u.Filepath()) if err != nil { - return 0, wrapfileError(u, err) + return 0, fileErr(err) } return info.Size(), nil } @@ -83,7 +82,7 @@ func (f *FileSystem) Exists(_ context.Context, u *URI) (bool, error) { return false, nil } if err != nil { - return false, wrapfileError(u, err) + return false, fileErr(err) } return true, nil } @@ -91,7 +90,7 @@ func (f *FileSystem) Exists(_ context.Context, u *URI) (bool, error) { func (f *FileSystem) List(ctx context.Context, u *URI) ([]Info, error) { entries, err := os.ReadDir(u.Filepath()) if err != nil { - return nil, wrapfileError(u, err) + return nil, fileErr(err) } infos := make([]Info, len(entries)) for i, e := range entries { @@ -127,9 +126,9 @@ func (f *FileSystem) checkPath(path string) error { return nil } -func wrapfileError(uri *URI, err error) error { +func fileErr(err error) error { if os.IsNotExist(err) { - return fmt.Errorf("%s: %w", uri, fs.ErrNotExist) + return fs.ErrNotExist } return err } @@ -144,7 +143,7 @@ var _ Sizer = (*fileSizer)(nil) func (f *fileSizer) Size() (int64, error) { info, err := os.Stat(f.uri.Filepath()) if err != nil { - return 0, wrapfileError(f.uri, err) + return 0, fileErr(err) } return info.Size(), nil } diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index 0bbf948985..990a4e7919 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -27,12 +27,12 @@ func NewS3() *S3Engine { func (s *S3Engine) Get(ctx context.Context, u *URI) (Reader, error) { r, err := s3io.NewReader(ctx, u.String(), s.client) - return r, wrapErr(err) + return r, s3Err(err) } func (s *S3Engine) Put(ctx context.Context, u *URI) (io.WriteCloser, error) { w, err := s3io.NewWriter(ctx, u.String(), s.client) - return w, wrapErr(err) + return w, s3Err(err) } func (s *S3Engine) PutIfNotExists(context.Context, *URI, []byte) error { @@ -40,21 +40,21 @@ func (s *S3Engine) PutIfNotExists(context.Context, *URI, []byte) error { } func (s *S3Engine) Delete(ctx context.Context, u *URI) error { - return wrapErr(s3io.Remove(ctx, u.String(), s.client)) + return s3Err(s3io.Remove(ctx, u.String(), s.client)) } func (s *S3Engine) DeleteByPrefix(ctx context.Context, u *URI) error { - return wrapErr(s3io.RemoveAll(ctx, u.String(), s.client)) + return s3Err(s3io.RemoveAll(ctx, u.String(), s.client)) } func (s *S3Engine) Size(ctx context.Context, u *URI) (int64, error) { info, err := s3io.Stat(ctx, u.String(), s.client) - return info.Size, wrapErr(err) + return info.Size, s3Err(err) } func (s *S3Engine) Exists(ctx context.Context, u *URI) (bool, error) { ok, err := s3io.Exists(ctx, u.String(), s.client) - return ok, wrapErr(err) + return ok, s3Err(err) } func (s *S3Engine) List(ctx context.Context, uri *URI) ([]Info, error) { @@ -72,7 +72,7 @@ func (s *S3Engine) List(ctx context.Context, uri *URI) ([]Info, error) { return infos, nil } -func wrapErr(err error) error { +func s3Err(err error) error { var reqerr awserr.RequestFailure if errors.As(err, &reqerr) && reqerr.StatusCode() == http.StatusNotFound { return fs.ErrNotExist