*: Fixed further not checked errors [PART2] #403
Conversation
b1848cc to
2733eeb
Compare
Makefile
Outdated
|
|
||
| all: install-tools deps format build | ||
| all: deps format build |
There was a problem hiding this comment.
did we not want errcheck to run locally as well? This is what i expect most people to run before putting out a PR.
pkg/objstore/objstore.go
Outdated
| // Best-effort cleanup. | ||
| if err != nil { | ||
| os.Remove(dst) | ||
| if rerr := os.Remove(dst); rerr != nil { | ||
| fmt.Println("failed to best effortly remove ", dst, "err:", rerr) |
There was a problem hiding this comment.
fmt ... why not pass logger into these funcs?
There was a problem hiding this comment.
Another thought ... do we want to add metrics around the failure to clean up?
There was a problem hiding this comment.
just wanted to avoid additional parameter, but I guess it might be necessary
There was a problem hiding this comment.
regarding metric.. I am not sure. I would keep metrics alert on bad symptomps, like "I cannot run further because I have some malformed dir". However most of processes are working on tmp dirs and removed on different levels. So this removal is only "nice" thing to have, nothing critical.
We definitely need to rething retry path and how to handle these. The issue #318 is for it.
There was a problem hiding this comment.
What about being SIMPLE here and be clear that the directory is malformed and can have partial download and caller should be responsible for clean up?
There was a problem hiding this comment.
Yeah, we should be using logger instead of print statement. Also you can remove "best effortly" substring.
Also add logger to other places too as we will eventually need it anyways as the program grows.
Makefile
Outdated
|
|
||
| all: install-tools deps format build | ||
| .PHONY: all | ||
| all: deps format build |
There was a problem hiding this comment.
add errcheck? I expect all people to run make before raising a PR so would catch it before CI.
pkg/objstore/s3/s3.go
Outdated
| @@ -200,7 +201,9 @@ func (b *Bucket) getRange(ctx context.Context, name string, off, length int64) ( | |||
| // NotFoundObject error is revealed only after first Read. This does the initial GetRequest. Prefetch this here | |||
| // for convenience. | |||
| if _, err := r.Read(nil); err != nil { | |||
| r.Close() | |||
| //runutil.LogOnErr(nil, r, "s3 get range obj close") | |||
There was a problem hiding this comment.
uncomment and remove the line below.
There was a problem hiding this comment.
yea, it was just for CI test - fixed now (:
There was a problem hiding this comment.
And checking the reviewer looked at the code 😛
pkg/objstore/objstore.go
Outdated
| // Best-effort cleanup. | ||
| if err != nil { | ||
| os.Remove(dst) | ||
| if rerr := os.Remove(dst); rerr != nil { | ||
| fmt.Println("failed to best effortly remove ", dst, "err:", rerr) |
There was a problem hiding this comment.
Yeah, we should be using logger instead of print statement. Also you can remove "best effortly" substring.
Also add logger to other places too as we will eventually need it anyways as the program grows.
pkg/testutil/prometheus.go
Outdated
| f, err := os.Create(filepath.Join(p.dir, "prometheus.yml")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer f.Close() | ||
| defer runutil.BestEffortErr(nil, &err, f, "prometheus config") |
There was a problem hiding this comment.
Use LogOnError, I think you care very little if you can't close prometehus.yml.
pkg/testutil/prometheus.go
Outdated
| @@ -146,7 +150,7 @@ func CreateBlock( | |||
| if err != nil { | |||
| return id, errors.Wrap(err, "create head block") | |||
| } | |||
| defer h.Close() | |||
| defer runutil.BestEffortErr(nil, &err, h, "TSDB Head") | |||
There was a problem hiding this comment.
Rename this function to CaptureErrIfNotSet and change comment to CaptureErrIfNotSet runs closer... Also change wrap argument name to format so that it's consistent with Sprintf. Also logger :P.
f7ef3ea to
c30a30e
Compare
| if strings.HasSuffix(name, DirDelim) { | ||
| return DownloadDir(ctx, bkt, name, filepath.Join(dst, filepath.Base(name))) | ||
| return DownloadDir(ctx, logger, bkt, name, filepath.Join(dst, filepath.Base(name))) |
There was a problem hiding this comment.
If DownloadDir fails, it should delete all files but you may end up with some empty directories. Is that ok?
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…hanos-io#403) Bumps [http-proxy-middleware](https://github.com/chimurai/http-proxy-middleware) from 2.0.4 to 2.0.7. - [Release notes](https://github.com/chimurai/http-proxy-middleware/releases) - [Changelog](https://github.com/chimurai/http-proxy-middleware/blob/v2.0.7/CHANGELOG.md) - [Commits](chimurai/http-proxy-middleware@v2.0.4...v2.0.7) --- updated-dependencies: - dependency-name: http-proxy-middleware dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Bartek Plotka bwplotka@gmail.com