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.

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

runutil: add Exhaust* fns, initial users #1302

Merged
merged 11 commits into from
Jul 22, 2019

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Jul 3, 2019

Changes

  • runutil package now has an extra family of functions which exhaust an io.ReadCloser before closing them
  • Internal Thanos code has been converted to use them where it makes sense i.e. resp.Body is used a.k.a. HTTP connections

I think that the old functions can still be there because we might have external users of this package and in some cases it just doesn't make sense to use them.

Rationale

Lets us reuse keep-alive connections efficiently. Ticket: #1206

@GiedriusS
Copy link
Member Author

@bwplotka do you agree with this direction? If yes, then I will convert the other users where this is applicable.

Copy link
Member

@bwplotka bwplotka 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 I am happy with that, yes.
Thanks! Some suggestions though.

pkg/runutil/runutil.go Outdated Show resolved Hide resolved
pkg/runutil/runutil.go Outdated Show resolved Hide resolved
@GiedriusS GiedriusS marked this pull request as ready for review July 9, 2019 12:38
@GiedriusS GiedriusS requested a review from bwplotka July 9, 2019 12:38
@GiedriusS
Copy link
Member Author

@bwplotka PTAL again when you'll have some time before merging. Thanks! 😄

Copy link
Member

@bwplotka bwplotka 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 we could potentially reduce number of methods here e.g by having just Close that accepts io.Closer but tries to cast to io.Reader and then optionally Copies? I think it is solid behavior. (:

What do you think? (:

@@ -108,6 +115,12 @@ func CloseWithLogOnErr(logger log.Logger, closer io.Closer, format string, a ...
level.Warn(logger).Log("msg", "detected close error", "err", errors.Wrap(err, fmt.Sprintf(format, a...)))
}

// ExhaustCloseWithLogOnErr closes the io.ReadCloser with a log message on error but exhausts the reader before.
func ExhaustCloseWithLogOnErr(logger log.Logger, r io.ReadCloser, format string, a ...interface{}) {
_, _ = io.Copy(ioutil.Discard, r)
Copy link
Member

Choose a reason for hiding this comment

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

Technically log on error might mean checking err here as well (: But I think it's fine for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true but such an error message would be harmless and users would just ignore it, no?

Copy link
Member

Choose a reason for hiding this comment

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

No if you want to exhaust and that failed, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but I'm not sure what an user could do. Plus, if that doesn't have any negative consequences besides just a little bit lower performance then I'm not sure that it's that important. I will add checking here as well, I just hope that it will not lead to unnecessary noise in logs 😄

@GiedriusS
Copy link
Member Author

I think we could potentially reduce number of methods here e.g by having just Close that accepts io.Closer but tries to cast to io.Reader and then optionally Copies? I think it is solid behavior. (:

What do you think? (:

It is a thing that we could do but the question here is do we want to force that behavior onto the external users of this package? IMHO it's not always useful to discard all of the input because you could never be sure what is actually in the Read() method of io.Reader.

@bwplotka
Copy link
Member

True you are right. (:

@GiedriusS GiedriusS requested a review from bwplotka July 19, 2019 12:39
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM!

@GiedriusS GiedriusS merged commit d02488d into thanos-io:master Jul 22, 2019
paulfantom added a commit to paulfantom/thanos that referenced this pull request Jul 26, 2019
* master:
  Fix typos (thanos-io#1350)
  pool: add pool raciness tests, fix p.usedTotal house-keeping (thanos-io#1340)
  runutil: add Exhaust* fns, initial users (thanos-io#1302)
  Further docs rename to new org; Improved docker login. (thanos-io#1344)
  Moved CI to thanos-io org and docker image to quay.io/thanos/thanos (thanos-io#1342)
  thanos-io (thanos-io#1343)
  website: migrate to new Github Org (thanos-io#1341)
paulfantom added a commit to paulfantom/thanos that referenced this pull request Jul 26, 2019
* master:
  Fix typos (thanos-io#1350)
  pool: add pool raciness tests, fix p.usedTotal house-keeping (thanos-io#1340)
  runutil: add Exhaust* fns, initial users (thanos-io#1302)
  Further docs rename to new org; Improved docker login. (thanos-io#1344)
  Moved CI to thanos-io org and docker image to quay.io/thanos/thanos (thanos-io#1342)
  thanos-io (thanos-io#1343)
  website: migrate to new Github Org (thanos-io#1341)
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