Semi-RFC: Consolidate http behavior #272

tiedotguy · 2019-10-24T01:35:37Z

This is the next change in the whole transport refactor saga, leading towards streaming (#10, #84, #159), InfluxDB (#14), and clustering. Nothing here is consumed currently, it's just the primitives. As it's getting rather large already, I wanted to minimize the review size. Commits are relatively stand-alone.

This PR is not expected to be merged yet, because there's documentation mismatches (due to lack of consumption). I'll be working on updating consumers in a subsequent PR, but didn't want to get too deep in to that in case there's feedback on the API (I'm not a huge fan of the TransportOptions for example)

ash2k · 2019-10-24T10:41:40Z

pkg/transport/client_low.go

+
+		// Base headers
+		req.Header.Set("Content-Type", contentType)
+		req.Header.Set("Content-Encoding", encoding)


Encoding and type could be enums for a little bit more type safety. Currently there is a risk to pass two strings in swapped positions.

Swapping is definitely a hazard, but I don't know how to prevent it. Go lacks enums (near as I can tell), and will happily promote a string to an alias of it, breaking the type safety: https://play.golang.org/p/lzAsJ8gGB_O

I could do a sanity check for valid values, but that seems overkill. Is there something I'm missing?

Yeah, what we are seeing in that example is the behavior of "untyped literals". This does not compile though because x is typed vs untyped literal. https://play.golang.org/p/wiCUwuQqj5g

Ahh! I see now. https://play.golang.org/p/xItV45OW49V

I'll fix it up in the cleanup commit (waiting on other feedback, I know the team is busy atm)

this

https://blog.golang.org/constants see the String constants section

ash2k · 2019-10-24T10:47:09Z

pkg/transport/helpers.go

+	if err != nil {
+		return nil, err
+	}
+	json.ReturnStream(stream)


So if there was an error we don't return the stream? This line should be after Flush?

Good catch!

this

ash2k · 2019-10-24T10:50:52Z

pkg/transport/pool.go

+	backoff, err := util.GetRetryFromViper(sub)
+	if err != nil {
+		tp.logger.WithField("name", name).WithError(err).Error("failed to load retry policy")
+		return nil, err


Is there a reason to both log and return the error? Ideally should only return it

Not specifically. Depending on the call stack it could actually end up dropping a few duplicate logs - they're all fatal though (fail early), so it's something I didn't put a lot of thought in to.

For preference I'd actually make it a .Fatal(). I've been slowly pushing to remove errors from the code (eg, this whole refactor is fire and forget) with a view to either fail-fast on startup, or handle runtime errors immediately when there's maximum context available.

Ultimately a lot of the error paths just pop out somewhere high level with a log line, and they lose context that could be supplied by an immediate log (such as the transport name in this case).

There's an argument to be made for more strongly typed errors to provide context, but a) I'm lazy, and b) the only way to expose that context is through error.Error() which means it's not exposed as structured log data.

Ok, I see your point. I think this code can be improved further if config parsing is moved into NewTransportPool() and viper is no longed referenced/used. I don't know if it's easy to do this. Also, just terminating the program in the middle of the pooling code would be weird. Testing and reusing this code becomes harder.

ash2k · 2019-10-24T10:51:51Z

pkg/transport/pool.go

@@ -80,16 +139,37 @@ func (tp *TransportPool) newClient(name string) (*Client, error) {
 		return nil, err
 	}

+	headers := map[string]string{}
+	headerKeys := []string{}


Could pre-allocate the exact size for both

ash2k · 2019-10-24T10:53:10Z

pkg/util/backoff.go

+	retryPolicy := v.GetString(paramRetryPolicy)
+
+	if retryInterval <= 0 {
+		return nil, errors.New(paramRetryInterval + " must be positive")


Could use fmt.Errorf() here and below for consistency with other places where things are not strings. Not a big deal.

My main thought was it should turn it in to a static string at compile time rather than runtime. I don't know if that will actually occur with either "" + "" or fmt.Sprintf("%s", ""), but that was the intent.

ash2k · 2019-10-24T10:55:08Z

pkg/util/semaphore_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestSemaphoreUnlimited(t *testing.T) {


t.Parallel()

tiedotguy · 2020-06-11T01:19:21Z

Rebased off master, fixed the issues mentioned. Last commit is a bit derpy because I did a fixup instead of reword when cleaning up the "address issues from review" commit.

This adds a richer retry policy framework configurable through Viper, allowing more centralisation and consistency.

There's also a null semaphore which lets anything in, which can be used to provide unbound access without extra logic.

…mplement high level constructs

MovieStoreGuy · 2020-07-07T05:32:20Z

Hey @tiedotguy , you currently have a merge conflict on this, would you be able to resolve this?

tiedotguy · 2020-07-07T05:33:51Z

No merge conflicts, simply happy accidents.

This branch will return later.

tiedotguy requested review from adampetrovic, ash2k, aelse and MovieStoreGuy October 24, 2019 01:35

ash2k reviewed Oct 24, 2019

View reviewed changes

tiedotguy mentioned this pull request Nov 11, 2019

Plan for clustering and streaming #277

Open

tiedotguy force-pushed the consolidate-http-behavior branch from d4cb6bc to e1cbdba Compare June 11, 2020 01:17

tiedotguy added 8 commits June 29, 2020 17:22

Cleanup messages and fix style from previous PRs

bf7ea59

Introduce a configurable retry policy framework.

05da854

This adds a richer retry policy framework configurable through Viper, allowing more centralisation and consistency.

Adds a basic semaphore which takes a context to allow for timeouts.

4ab89ec

There's also a null semaphore which lets anything in, which can be used to provide unbound access without extra logic.

Adds basic low level HTTP POSTing with retries to the transport.

d6e55e5

Split transport client in to low level / high level constructs, and i…

ece26ee

…mplement high level constructs

Send transport metrics

51a59f9

Fix NPE in test

2c66d5f

Fix custom header description

c24cca1

tiedotguy force-pushed the consolidate-http-behavior branch from 7fd4aeb to c24cca1 Compare June 29, 2020 07:25

tiedotguy closed this Jul 7, 2020

MovieStoreGuy reopened this Jul 7, 2020

MovieStoreGuy closed this Jul 7, 2020

tiedotguy deleted the consolidate-http-behavior branch October 5, 2020 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semi-RFC: Consolidate http behavior #272

Semi-RFC: Consolidate http behavior #272

tiedotguy commented Oct 24, 2019

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019

ash2k Oct 25, 2019

tiedotguy Oct 25, 2019 •

edited

Loading

ash2k Oct 25, 2019

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019 •

edited

Loading

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019

ash2k Oct 25, 2019

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019 •

edited

Loading

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019

ash2k Oct 24, 2019

tiedotguy Oct 25, 2019 •

edited

Loading

tiedotguy commented Jun 11, 2020

MovieStoreGuy commented Jul 7, 2020

tiedotguy commented Jul 7, 2020

Semi-RFC: Consolidate http behavior #272

Semi-RFC: Consolidate http behavior #272

Conversation

tiedotguy commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiedotguy Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiedotguy Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiedotguy Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiedotguy Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

tiedotguy commented Jun 11, 2020

MovieStoreGuy commented Jul 7, 2020

tiedotguy commented Jul 7, 2020

tiedotguy Oct 25, 2019 •

edited

Loading

tiedotguy Oct 25, 2019 •

edited

Loading

tiedotguy Oct 25, 2019 •

edited

Loading

tiedotguy Oct 25, 2019 •

edited

Loading