These are chat archives for paypal/squbs

28th
Mar 2017
Anil Gursel
@anilgursel
Mar 28 2017 14:56
Hmmm.. looks like your PR build failed because of some Circuit Breaker related test failures.. Probably timing issues..
Anil Gursel
@anilgursel
Mar 28 2017 15:04

@akara Regarding integrating Circuit Breaker to ClientFlow, the API currently looks like flow:

  def apply[T](name: String,
              connectionContext: Option[HttpsConnectionContext] = None,
              settings: Option[ConnectionPoolSettings] = None,
              circuitBreakerSettings: Option[CircuitBreakerSettings[HttpRequest, Try[HttpResponse], T]] = None,
              env: Environment = Default)(implicit system: ActorSystem, fm: Materializer): ClientConnectionFlow[T] = {

However, we allow it to be configured through application.conf as well, which is pretty easy for the state, e.g.,

myclient {
  type = squbs.httpclient

  circuit-breaker {
    circuit-breaker {
    max-failures = 5
    call-timeout = 50 ms
    reset-timeout = 5 seconds
    max-reset-timeout = 1 minute
    exponential-backoff-factor = 1.0
  }
}

But, the problem is with fallback, failureDecider and uniqueIdMapper. It does not make sense “some” to be provided through configuration and “some” through application.conf. I wonder if I should provide traits to provide these and point from the configuration, e.g.,

trait Fallback[In, Out, Context] {
  def fallback: ((In, Context)) => (Try[Out], Context)
}

What do you think ?

Need to do this for all three and provide japi versions..
Thinking if there is any better, more elegant way to tackle this.
Akara Sucharitakul
@akara
Mar 28 2017 16:02
That's definitely one option.
Wondering if these need to be controlled by application.conf.
And whether we should have one trait for each functionality, or one for all.
Anil Gursel
@anilgursel
Mar 28 2017 16:05
if we do not control it by application.conf, then should we let any circuit breaker configuration to be controlled by application.conf, e.g., max-failures? If we do, then, how do we deal with partial configurations? If a CircuitBreakerSettings is passed in as a parameter, then all settings should be there and no application.conf?
and application.conf comes into picture when there is no CircuitBreakerSettings (which is anyway the desired scenario), but if a user needs to specify fallback, he/she always need to use programmatic API ?
Akara Sucharitakul
@akara
Mar 28 2017 16:07
There are certain areas that are a good fit for application.conf. Certain areas are not such a good fit, like when you need to pass functions. Thinking...
Anil Gursel
@anilgursel
Mar 28 2017 16:10
Actually, I prefer keeping it simple.. No fallback, failureDecider, and uniqueIdMapper in application.conf, if you need to configure any of these, you need to go programmatic API, with all settings coming from programmatic API..
Akara Sucharitakul
@akara
Mar 28 2017 16:11
I was thinking about that, too. But then if the user desires to read part of that from the config, it will not provide any standard place/structure.
Another option is to separate these into two parts. One is the config, one are the handlers. The handlers need to be provided programmatically, the config can be provided programmatically or through conf file.
Akara Sucharitakul
@akara
Mar 28 2017 16:16
Or, go with your suggestion forcing the programmatic API but provide a helper to populate the values from config.
Anil Gursel
@anilgursel
Mar 28 2017 16:19

that makes things complicated though, IMO.. Just for clarification though, I am talking in the context of http client.. For the standard CircuitBreakerSettings api, we already have the partial config mechanism..

case class CircuitBreakerSettings[In, Out, Context] private[circuitbreaker] (
  circuitBreakerState: CircuitBreakerState,
  fallback: Option[((In, Context)) => (Try[Out], Context)] = None,
  failureDecider: Option[((Try[Out], Context)) => Boolean] = None,
  uniqueIdMapper: Context => Option[Any] = (_: Any) => None) {

AtomicCircuitBreakerState has a way to initializee from configuration.. and for others no way.. but, the user needs to do

val cbState = AtomicCircuitBreakerState(“myCb”,  config)
val cbSettings = CircuitBreakerSettings(cbState, fallbackfunction, failureDeciderFunction, uniqueIdMapper)
Akara Sucharitakul
@akara
Mar 28 2017 16:21
So the config above is used by AtomicCircuitBreakerState?
Anil Gursel
@anilgursel
Mar 28 2017 16:21

For http client, we pass an optional CircuitBreakerSettings.. I do not want to treat CircuitBreakerSettings any different than other parameters.. For all parameters, if it is provided honor the parameter, otherwise try to initialize it yourself..

Only for CircuitBreakerSettings, try to intiailizer from application.conf as well, kinda makes it confusing IMO

So the config above is used by AtomicCircuitBreakerState?

yes

Akara Sucharitakul
@akara
Mar 28 2017 16:23
How do you pass the CircuitBreakerState from the ClientFlow API above?
Anil Gursel
@anilgursel
Mar 28 2017 16:23
ClientFlow API takes an option CircuitBreakerSettings, not directly a CircuitBreakerState:
def apply[T](name: String,
              connectionContext: Option[HttpsConnectionContext] = None,
              settings: Option[ConnectionPoolSettings] = None,
              circuitBreakerSettings: Option[CircuitBreakerSettings[HttpRequest, Try[HttpResponse], T]] = None,
              env: Environment = Default)(implicit system: ActorSystem, fm: Materializer): ClientConnectionFlow[T]
Akara Sucharitakul
@akara
Mar 28 2017 16:24
So the state is part of the settings?
Anil Gursel
@anilgursel
Mar 28 2017 16:24
yes
Akara Sucharitakul
@akara
Mar 28 2017 16:24
Same instance of settings, same instance of state?
Anil Gursel
@anilgursel
Mar 28 2017 16:26
settings includes
  • state includes
    • name
    • max-failures
    • callTimeout
    • resetTimeout
    • maxResetTimeout
    • exponentialBackoffFactor
  • fallback
  • failureDecider
  • uniqueIdMapper
Akara Sucharitakul
@akara
Mar 28 2017 16:28
I start to see the pattern separating here. The hard configurables are actually at the state layer.
The settings themselves are a bunch of functions. Not good for conf files.
Anil Gursel
@anilgursel
Mar 28 2017 16:29
Yes.
Akara Sucharitakul
@akara
Mar 28 2017 16:30
So if you don't provide the settings, no way to configure fallback, failureDecider, etc.
If you provide it, the only thing we care about is the state can read the config. So, instantiating the state can be done with the config.
Anil Gursel
@anilgursel
Mar 28 2017 16:31
True. But, for http client, I am planning to have a default failureDecider if not provided.. But, we can discuss about this.. Success response with Internal Server Error
Akara Sucharitakul
@akara
Mar 28 2017 16:33
Default values for the settings? Option?
Anil Gursel
@anilgursel
Mar 28 2017 16:33
fallback and failureDecider are option. There is a default function for uniqueIdMapper. state is required.
Akara Sucharitakul
@akara
Mar 28 2017 16:34
That sounds good. So the CircuitBreakerSettings will have it accordingly.
Anil Gursel
@anilgursel
Mar 28 2017 16:34
we already have a way to initialize state from configuration.. And the current logic is that.. If CircuitBreakerSettings is a None, then initialize AtomicCircuitBreakerState from configuration and create as follows:
CircuitBreakerSettings(stateFromConfig, None, defaultHttpFailureDecider)
Akara Sucharitakul
@akara
Mar 28 2017 16:35
Sounds good.
Looks like we have a call. Or are we still not clear?
Anil Gursel
@anilgursel
Mar 28 2017 16:36
let me summarize it here to see if we are on the same picture..
  • If Some(circuitBreakerSettings) is provided use it exclusively, do not read application.conf for anything.
  • If circuitBreakerSettings is a None, then initialize AtomicCircuitBreakerState from configuration. And initialize CircuitBreakerSettings as follows:
     CircuitBreakerSettings(stateFromConfig)
  • If failureDecider in CircuitBreakerSettings is a None, then use default http failure decider (which is considering 5xx as failure).
Anil Gursel
@anilgursel
Mar 28 2017 16:42
Let me know if we are on the same picture.. I am just not sure about the last item. User is passing None, and we are overriding it with default.. I feel it is ok but not 100% sure :D
Akara Sucharitakul
@akara
Mar 28 2017 16:44
I'm totally good with the last item.
The only addition is under bullet point 1, I want to get AtomicCircuitBreakerState from configuration, I should be able to do so without much difficulty.
Anil Gursel
@anilgursel
Mar 28 2017 16:52
CircuitBreakerSettings requires a CircuitBreakerState. We cannot make it an Option. If it is not an Option, if a CircuitBreakerSettings is provided with a CircuitBreakerState then we should not attempt to override it from configuration.
Akara Sucharitakul
@akara
Mar 28 2017 16:53
That was not my point. My point was you do provide a AtomicCircuitBreakerState, but read its config from conf file.
Anil Gursel
@anilgursel
Mar 28 2017 16:54
yeah, that’s already there:
object AtomicCircuitBreakerState {
  /**
    * Create a new Circuit Breaker State from configuration.
    *
    * @param name The unique name of this circuit breaker instance.
    * @param config Configuration to look for the settings
    * @param system ActorSystem
    */
  def apply(name: String, config: Config)(implicit system: ActorSystem): CircuitBreakerState
Akara Sucharitakul
@akara
Mar 28 2017 16:54
:+1:
Then we're good.
Anil Gursel
@anilgursel
Mar 28 2017 16:54
Great!
thanks for the inputs.
Akara Sucharitakul
@akara
Mar 28 2017 16:54
Thank you for going through all the trouble!
Anil Gursel
@anilgursel
Mar 28 2017 16:55
no trouble actually. I just would like to agree on the design before I spend too much time on it :)
Anil Gursel
@anilgursel
Mar 28 2017 17:14
@akara I see that I had defined fallback as ((In, Context)) => (Try[Out], Context).. Does the fallback function really need to know about the Context? Same goes for failureDecider..
Akara Sucharitakul
@akara
Mar 28 2017 17:36
I would keep it in place.
@anilgursel How strong do you feel about AbstractCustomRouteTestKit vs CustomRouteTestKit? I feel the former is a bit long-ish, for consistency. But it's not that I'd insist on the shorter one.
Anil Gursel
@anilgursel
Mar 28 2017 17:39

IMO, it makes it more difficult to provide a fallback function.. I mean the function looks less natural, as most of the cases Context is not used.. Also, API looks more complicated..

But, another things is.. It makes it difficult in ClientFlow integration.. Because, the user should deal with T, but internall we do RequestContext as the Context.. The mappings get challenging.

From Akka HTTP client point of view Context is opaque. The value of Try[HttpResponse] should not depend on Context..
Akara Sucharitakul
@akara
Mar 28 2017 17:42
We're assuming the context to be a simple object. But what if it contains information used for making this decision that may not be in the request itself? Would the user want to have access to the context?
Anil Gursel
@anilgursel
Mar 28 2017 17:43
I think it complicates the logic and design. Those should be isolated and separate concerns.
Akara Sucharitakul
@akara
Mar 28 2017 17:43
The only other way for users to deal with it is to pass custom headers in the request. I know we've done that but I'm not sure it is always the right thing.
The question is... by how much?
Anil Gursel
@anilgursel
Mar 28 2017 17:45
Is it right to depend on the context for the actual request/response cycle.. Response should be determined only by a function of request.
We do not pass context over the wire.
Akara Sucharitakul
@akara
Mar 28 2017 17:45
I'm thinking if the context has something like request attributes as part of it, it may become significant. Unless the API gets really messy, I'd not take it away.
Anil Gursel
@anilgursel
Mar 28 2017 17:46
IMO, our API gets messy. And the app code gets more spagetti-ish as well.
  • our integration logic gets messy as well. Which I expect other CB integrations will get messy as well.
Akara Sucharitakul
@akara
Mar 28 2017 17:46
Again, we have been dealing with custom headers that could not carry anything but a String.
Anil Gursel
@anilgursel
Mar 28 2017 17:48
why do we need to carry those to fallback and failureDecider ?
Akara Sucharitakul
@akara
Mar 28 2017 17:48
I'm not yet too convinced on the messy part.
Anil Gursel
@anilgursel
Mar 28 2017 17:49
“messy” may not be right word. I think “complicated” is a better one.
and IMO, unccessarily complicated.
Akara Sucharitakul
@akara
Mar 28 2017 17:51
Since those are functions, they need to make a call what they want to return. The failureDecider is the only one I feel may not need the context. However, if you decide to add request attributes to the context, then they can well be used to figure what the fallback action is. Of course, you can always make the argument of having the user dealing with passing such attributes as headers. IMO that just pushes the responsibility to the user.
Question is, does it make our life harder or the users' life harder?
Anil Gursel
@anilgursel
Mar 28 2017 17:51
providing capability is not the right option always. We should not leak data all over. that leads to difficult to follow designs. Context should be just the Context for ordering. This is what Akka HTTP uses it for and we should stick to it as much as possible.
Akara Sucharitakul
@akara
Mar 28 2017 17:52
Akka HTTP is not even using it. It is just allowing you to identify the request.
Anil Gursel
@anilgursel
Mar 28 2017 17:52
exactly..
that’s what we should try to do as much as possible. We should not add more meaning/capability to Context. we already do it for Circuit Breaker uniqueness identification.. and we should limit it to that.
Akara Sucharitakul
@akara
Mar 28 2017 17:53
But, since we pass functions that may need a context to make decisions, should we prohibit it?
We're not saying they have to. But it is a question about enabling or not enabling the user to do so.
Anil Gursel
@anilgursel
Mar 28 2017 17:54
The context it needs should be HttpRequest, IMO. From HttpRequest it should create a reponse, or create a new HttpRequest to call another service.
But, I think my last message says that if we need to call another service we need to pass the context :smile:
Akara Sucharitakul
@akara
Mar 28 2017 17:55
And we don't make use of the context beyond our needs. Users won't make use of it beyond their needs.
I really won't use the CircuitBreaker fallback to make another service call. I'd rather not have it fallback and make a service call in the flow.
Basically deciding outside the given fallback.
Anil Gursel
@anilgursel
Mar 28 2017 17:56
but, that’s one of the main use cases of CircuitBreaker.
If a service is not responding, try another one..
Akara Sucharitakul
@akara
Mar 28 2017 17:57
So you have to materialize a new flow inside the fallback?
Anil Gursel
@anilgursel
Mar 28 2017 17:57
if we do not do it automatically, a significant value is lost.
I understand your point. But, should we prohibit it? If so, then I do not see a point in passing Context to fallback