circuit: introduce half_open_resource_timeout

[sirupsen]

When a circuit is busy (for those at Shopify, see https://github.com/Shopify/shopify/pull/164168) you may reduce a substantial amount of capacity if you have large timeouts (see #145). One way to circumvent this is to have very large error_threshold timeouts, as a multiple of your timeout. E.g. if your resource timeout is 30s, then an error_timeout of 30s will make sure you have 50% of capacity available, and 60s will make sure you have 25% available (assuming that the circuits trigger equally over the timeout period, which is not an unreasonable assumption in production). However, this has obvious drawbacks, making recovering from incidents take longer for the affected resource.

Instead, we can reduce the resource timeout in the half_open? state to achieve something similar. Normally, timeouts are to allow outlier queries to go through—in the face of an incident, we can eat that (we'd want to kill those anyway) and reduce the timeout dramatically while half_open? on the circuit.

The ugly thing about this, is that it couples the circuit with the resource. However, I think it makes more sense for the circuit to know about the resource, than for the resource to know about the circuit—that's why I chose to implement it this way.

This will need to be implemented per adapter, by implementing with_resource_timeout.

cc @Shopify/servcomm

[sirupsen]

This is the coupling I think is ugly; passing in the resource and figuring out whether it quacks correctly here. When I do a 2nd iteration here with comments, I'll likely refactor to pass the raw resource into the initializer of the bulkhead and circuit—which seems a bit more clean.

[sirupsen]

Ugh... for some reason this didn't surface in local tests, and only fails in one place on CI, but sometimes it won't allow reconfiguring the timeouts for an established client 😖

Found this... brianmario/mysql2#317 does not look promising... Kir worked on this in brianmario/mysql2#955, I'll talk to him and see if we can get this in.

[casperisfine]

I like the idea, and I think it makes sense to only give a small timeout when going into half-open to check if the resource is properly recovered.

However I'm afraid that queries that can't possibly be executed with this reduced timeout might cause the CB to stay open longer than it should.

[casperisfine]

Should we support different timeouts for read / write, or is it assumed this timeout is simply for the recovery phase so there is no need for it to be fine grained?

[casperisfine]

/nitpick, pass &block to safe one stack frame and it's nicer.

[sirupsen]

I read the MySQL2 source, and we can get the read_timeout without https://github.com/brianmario/mysql2/pull/955—so let's do that for now.

This should be good for final review.

[csfrancis]

Curious why you had to change this?

[sirupsen]

I upped the default timeout to 2s, to show that it changes to 1s during the half open state.

[casperisfine]

Implementation is 👍 to me.

Still unsure about the actual effect in prod, but I'm sure you'll look at those closely so 👍 as well.

[pushrax]

In general this pattern makes me a bit uncomfortable when expressed like this. If someone were to add a line of code to the top of this method that might raise, the ensure could set a nil value. An alternative that more clearly maps to the intention is:

def
  prev = current
  begin
    current = temp
    # things
  ensure
    current = prev
  end
end

[kirs]

@sirupsen have you looked into how hard it would be to make other adapters (e.g. Redis) to support the same with_resource_timeout trick?

[sirupsen]

@kirs pretty easy, I just decided to prioritize MySQL here because those resources don't generally pose larger infra concerns in terms of occupying capacity because of their lower timeouts. For MySQL, it wouldn't be crazy to have this 25-50x as small. For something like Redis, you want to never really allow any query to take more than ~250ms. You're not going to get an order of magnitude there, but it might be worth it eventually.