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

Add retry to external request API #163

Merged
merged 5 commits into from
Jan 2, 2019
Merged

Add retry to external request API #163

merged 5 commits into from
Jan 2, 2019

Conversation

gfr10598
Copy link
Contributor

@gfr10598 gfr10598 commented Jan 2, 2019

This adds retry for ServiceUnavailable, and latency and type metrics. This provides a simple external http request wrapper api that can be compiled into ETL (or other clients).


This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 2, 2019

Pull Request Test Coverage Report for Build 1544

  • 49 of 67 (73.13%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0%) to 46.807%

Changes Missing Coverage Covered Lines Changed/Added Lines %
handler/handler.go 0 7 0.0%
api/v2/api-v2.go 49 60 81.67%
Totals Coverage Status
Change from base Build 1483: 1.0%
Covered Lines: 645
Relevant Lines: 1378

💛 - Coveralls

@gfr10598 gfr10598 requested a review from pboothe January 2, 2019 15:08
Copy link
Contributor

@pboothe pboothe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


api/v2/api-v2.go, line 60 at r1 (raw file):
Some curious rounding choices here.

According to:

python
Python 2.7.13 (default, Nov 24 2017, 17:33:09) 
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print ", ".join([ "%2.0f" % 10**(1+i/10.0) for i in range(10) ])
10, 13, 16, 20, 25, 32, 40, 50, 63, 79

The right sequence is [10, 13, 16, 20, 25, 32, 40, 50, 63, 79] or (with some rounding to "nice" numbers) [10, 12, 15, 20, 25, 32, 40, 50, 64, 80]

(sorry for going down the rabbit hole on this one - the "30, 40, 48" subsequence caught my eye as obviously wrong)


api/v2/api-v2.go, line 95 at r1 (raw file):

		start := time.Now()
		resp, err := post(ctx, url, encodedData)
		switch {

Convert to if for readability due to the internal select. Or some other way of avoiding having case and default mean two different things on near-adjacent lines.


api/v2/api-v2.go, line 107 at r1 (raw file):

				RequestTimeHistogram.WithLabelValues(ctx.Err().Error()).Observe(float64(time.Since(start).Nanoseconds()) / 1e6)
				return nil, ctx.Err()
			default:

This way of doing things means that, with high probability, canceling the context will cause one last spurious call to the service. Is that okay? If it is not, please refactor to case <-time.After(1 * time.Second):


api/v2/api-v2.go, line 133 at r1 (raw file):

		defer httpResp.Body.Close()
		if err == ErrStatusNotOK {
			body, err2 := ioutil.ReadAll(httpResp.Body)

call it ioerr or errio or errIO or errioutil please.


api/v2/api-v2.go, line 137 at r1 (raw file):

				return nil, err2
			}
			if len(body) > 60 {

I don't understand this 60. Maybe add a comment? It seems like it's just truncating potentially overlong error messages at an arbitrary point - if it is that, add a comment to that effect.


api/v2/api-v2_test.go, line 28 at r1 (raw file):

	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		switch {

an if would be fewer lines, unless you anticipate new error types appearing. (this comment is non-blocking)


handler/handler.go, line 278 at r1 (raw file):

		response, err = AnnotateV2(request.Date, request.IPs)
		if err != nil {
			switch err {

an if would be fewer lines, unless you anticipate new error types appearing. (this comment is non-blocking)


handler/handler.go, line 283 at r1 (raw file):

			default:
				w.WriteHeader(http.StatusInternalServerError)
			}

Please test this new behavior.

@pboothe
Copy link
Contributor

pboothe commented Jan 2, 2019

Several of small comments. Overall it looks good to me, but I requested enough changes that it felt weird to just click "LGTM", because lots of small changes can spur refactorings which makes things into large changes, etc.

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


api/v2/api-v2.go, line 60 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Some curious rounding choices here.

According to:

python
Python 2.7.13 (default, Nov 24 2017, 17:33:09) 
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print ", ".join([ "%2.0f" % 10**(1+i/10.0) for i in range(10) ])
10, 13, 16, 20, 25, 32, 40, 50, 63, 79

The right sequence is [10, 13, 16, 20, 25, 32, 40, 50, 63, 79] or (with some rounding to "nice" numbers) [10, 12, 15, 20, 25, 32, 40, 50, 64, 80]

(sorry for going down the rabbit hole on this one - the "30, 40, 48" subsequence caught my eye as obviously wrong)

Done.


api/v2/api-v2.go, line 95 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Convert to if for readability due to the internal select. Or some other way of avoiding having case and default mean two different things on near-adjacent lines.

Done.


api/v2/api-v2.go, line 107 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

This way of doing things means that, with high probability, canceling the context will cause one last spurious call to the service. Is that okay? If it is not, please refactor to case <-time.After(1 * time.Second):

Done.


api/v2/api-v2.go, line 133 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

call it ioerr or errio or errIO or errioutil please.

Done.


api/v2/api-v2.go, line 137 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

I don't understand this 60. Maybe add a comment? It seems like it's just truncating potentially overlong error messages at an arbitrary point - if it is that, add a comment to that effect.

Done.


handler/handler.go, line 278 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

an if would be fewer lines, unless you anticipate new error types appearing. (this comment is non-blocking)

Done


handler/handler.go, line 283 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Please test this new behavior.

It looks like this is difficult to test until we get a bunch of annotator cache stuff (from my big prototype PR) in place. I'm adding a TODO, as this is fairly important functionality for the retry logic.

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


api/v2/api-v2_test.go, line 28 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

an if would be fewer lines, unless you anticipate new error types appearing. (this comment is non-blocking)

Done

Copy link
Contributor

@pboothe pboothe left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, 1 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


handler/handler.go, line 283 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

It looks like this is difficult to test until we get a bunch of annotator cache stuff (from my big prototype PR) in place. I'm adding a TODO, as this is fairly important functionality for the retry logic.

TODO in the test file with 201901 target date.

@gfr10598 gfr10598 merged commit a8d8180 into master Jan 2, 2019
@gfr10598 gfr10598 deleted the retry branch January 2, 2019 22:22
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.

3 participants