These are chat archives for paypal/squbs

24th
Mar 2017
Akara Sucharitakul
@akara
Mar 24 2017 06:09
Sorry for getting back so late. Generally, the whole idea is definitely correct. We should not do Try[Try[HttpResponse]] in any case. One point I'm still debating in my mind is whether pipeline atop circuitbreaker ... or circuitbreaker atop pipeline .... Both are valid cases and have their benefits/drawbacks. Pipeline allows you to collect stats. But the circuitbreaker atop also allows you to short-circuit sooner.
BTW, you don't need to do _ @ (Success(... in the pattern match. Just (Success(... is good enough. We don't care about the value of that _ anyway.
Another thought - cant we just .map { case (t, c) => (t.flatten, c) } instead of doing the pattern match? What outcome would be different?
Akara Sucharitakul
@akara
Mar 24 2017 06:17
Also, thanks a ton for merging #436
Anil Gursel
@anilgursel
Mar 24 2017 14:36
Yes, I think I added _ @ to make sure on the type matching. But, yes, I can try to dro.. I’ll try flatten..
I think we should do pipeline atop circuitbreaker join httpflow, because a user can always disable Circuit Breaker at http client level, and manually wrap the flow manualCircuitBreaker atop pipeline join httpflow.
from tracevbility, logging and metrics perspective, I prefer circuit breaker to be closer to the http client..
Akara Sucharitakul
@akara
Mar 24 2017 15:07
I won't feel strongly about either way at this point. Need to consider the pros/cons. For instance, pipeline atop circuitbreaker will show you stats for failed calls due to open circuit breaker.
We need to make sure they show up correctly.
Anil Gursel
@anilgursel
Mar 24 2017 16:18
yes, I actually would like to see the stats for failed calls.. and logging etc..
yes, we need to make sure they show correctly..
another thing to consider is that.. if we do pipeline atop circuitbreaker; then user can always disable circuit breaker at the http client level.. and use circuit breaker manually to do circuitbreaker atop pipeline..
but not the other way around..
Akara Sucharitakul
@akara
Mar 24 2017 16:21
Sounds good. The only ask then is the reporting for CB open stats be in its own category when viewed from the pipeline.
Anil Gursel
@anilgursel
Mar 24 2017 16:40
we already report each exception separately.. So, fail fast or timeout will already automatically be reflected.. However, for a fallback, it would show as regular.. no distinction.. But, we already have a JMX bean for circuit breaker stats, which should show all the counts, fail fast, fallback etc. Accordingly, I think we’re covered there.
Akara Sucharitakul
@akara
Mar 24 2017 16:41
:+1:
And noticed the iterator generates Ints. Unlike Long, int's wrap around much quicker. Do you see any issues?
We use these for ordering afterwards. What happens in a wrap around?
Akara Sucharitakul
@akara
Mar 24 2017 16:47
At 10,000 TPS it is slightly less than 60 hours.
Now this is for each materialization, which means for each connection. Do you see a chance a single connection will be used at 10,000 TPS for 60 hours straight? (Or make it 1,000 TPS for 600 hours on a single connection.
If not sure, we better take the action to make it a Long. We'll surely have many years to go.
Anil Gursel
@anilgursel
Mar 24 2017 17:03
I think we should change it to Long
Anil Gursel
@anilgursel
Mar 24 2017 19:15

@akara we already provide to setup circuit breaker with parameters.. https://github.com/paypal/squbs/blob/master/docs/circuitbreaker.md#scala

I am adding to pass a Config object.. Do you think we should allow both.. or just Config ?

Akara Sucharitakul
@akara
Mar 24 2017 21:05
At this time, a Config object would be more sensible, I think. Passing parameters could make your client initialization a bit long. What do you think?
Anil Gursel
@anilgursel
Mar 24 2017 21:06
passing parameters is the current API.. I feel like dropping it and just passing Config..
not necessarily because it is long list of parameters, but to keep less APIs..
Akara Sucharitakul
@akara
Mar 24 2017 21:06
So changing the current API?
Anil Gursel
@anilgursel
Mar 24 2017 21:08
yeah.. I am little mixed.. The Akka circuit breaker allows passing long list of parameters.. which we have today.. I am thinking of adding Config as a parameter, and dropping the list of parameters.. but, that would cause it to move away from what folks used to in Akka world.. not sure..
Also, we also have a way of initializing from configuraton by just passing the name, and it reads from the config .. https://github.com/paypal/squbs/blob/master/docs/circuitbreaker.md#creating-circuitbreakerstate-from-system-configuration. Having this along with passing the Config object.. not sure if it is causing more confusion..
I can have all three APIs.. Or I can choose 2 out of 3, where one of them needs to be Config based..
Akara Sucharitakul
@akara
Mar 24 2017 21:11
1) list of parameters, 2) name, 3) Config, right?
Anil Gursel
@anilgursel
Mar 24 2017 21:12
yes

Another question.. Here is the new ClientFlow API looks like:

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

How many combinations should we providing for Java create API ? Currently, there are three https://github.com/paypal/squbs/blob/master/squbs-httpclient/src/main/scala/org/squbs/httpclient/ClientFlow.scala#L50. I would always keep cb settings together.. But, not sure, if I should do all of the below combinations:

  • name
  • name, connectionContext & settings
  • name, connectionContext & settings, circuitBreakerState & fallback
  • name, circuitBreakerState & fallback
  • name, circuitBreakerState & fallback, env
  • name, connectionContext & settings, circuitBreakerState & fallback, env
Akara Sucharitakul
@akara
Mar 24 2017 21:12
The extra name does not make too much sense to me as you'd read config by client name.
Anil Gursel
@anilgursel
Mar 24 2017 21:14
I tend to agree.. So, eliminating type = squbs-circuitbreaker..
Let’s do 1 & 3 then.
Akara Sucharitakul
@akara
Mar 24 2017 21:14
You mean from the standalone CircuitBreaker as well?
Anil Gursel
@anilgursel
Mar 24 2017 21:15
yes, I am talking about standalone circuit breaker. Nothing above is http related..
Akara Sucharitakul
@akara
Mar 24 2017 21:15
The Java API combinations call for a Builder in Java.
Anil Gursel
@anilgursel
Mar 24 2017 21:15
I agree.. Builder makes sense..
:+1:
Akara Sucharitakul
@akara
Mar 24 2017 21:16
OK. Eliminating type = squbs-circuitbreaker
Users can manage the reading of that.
Anil Gursel
@anilgursel
Mar 24 2017 21:17
I agree!
One more question.. We need a name for circuit breaker.. The current APIs take a name for all.. For the parameter list one, I pass it as another parameter.. For the config one, should I do name, config or name should come from Config, like any other setting max-failures etc.. ?
Akara Sucharitakul
@akara
Mar 24 2017 21:20
If name comes from config, that config is not re-usable across multiple circuitbreakers.
Anil Gursel
@anilgursel
Mar 24 2017 21:20
that’s true.
Ok, name is a separate parameter, which makes sense..
Akara Sucharitakul
@akara
Mar 24 2017 21:21
:+1:
Anil Gursel
@anilgursel
Mar 24 2017 21:24

Also, I am planning to keep a default configuration of

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

and reference to it from the code.. and use it as a fallback when all are not provided.. I think it kinda matches with Akka HTTP configuration setting as well, e.g., ConnectionPoolSettings…. what do you think ?

Akara Sucharitakul
@akara
Mar 24 2017 21:24
I like it.
Anil Gursel
@anilgursel
Mar 24 2017 21:24
:+1: