These are chat archives for paypal/squbs

8th
Feb 2017
Akara Sucharitakul
@akara
Feb 08 2017 00:00
I have another mtg now and will look at #375 after.
Anil Gursel
@anilgursel
Feb 08 2017 00:00
thanks!
Anil Gursel
@anilgursel
Feb 08 2017 00:06
between, I had dropped some comments on paypal/squbs#378 yesterday. This is already a merged PR; my comments are especially around documentation.
also, I just started working on paypal/squbs#342. I will make it registered at the squbs-httpclient level by default (not at template level).
Anil Gursel
@anilgursel
Feb 08 2017 00:19
actually, I cannot do that at the squbs-httpclient level as it cannot have ExtensionLifecycle, as it is unaware of unicomplex
Anil Gursel
@anilgursel
Feb 08 2017 00:33
@akara It looks like I need to define DefaultHttpEndpointResolver in squbs-httpclient; however, need to register it at the template level. It would be great if it got registered at squbs-httpclient level. One way I can try is to make findResolver in ResolverRegistryExtension accessible, then call it inside CilentFlow, if it cannot find it, register the default one. But, that also goes against the overall design. We should let teams to unregister a resolver if you want to. We cannot force a special resolve to be always there.
Akara Sucharitakul
@akara
Feb 08 2017 00:35
In that case would the template be the best solve?
Akara Sucharitakul
@akara
Feb 08 2017 01:24
Done with review. Just one note on the naming. Otherwise all good.
Anil Gursel
@anilgursel
Feb 08 2017 15:06
@akara I would actually prefer the resolver be self contained in squbs-httpclient; however, the solutions there won’t be clean. So, doing it at the template level might be ok for now. However, we also need to think about the scenario where squbs-httpclient is used outside of the squbs ecosystem (as it only needs an actor system). In such scenario, the very first experience would be invalid endpoint resolution, which I really do not like..
Anil Gursel
@anilgursel
Feb 08 2017 15:19
@akara Replied to your comment on github regarding the checkAndRecordShortCircuit() name change. Your point is very valid; however, I am not very sure about the method name including something with metric recording.
How about the following method names?
  • markSuccess
  • markFailure
  • checkAndMarkIfShortCircuit
Akara Sucharitakul
@akara
Feb 08 2017 17:27
@anilgursel That sounds better. It should infer this call involves state.
Anil Gursel
@anilgursel
Feb 08 2017 17:27
Ok, changing all three methods names as above then
Akara Sucharitakul
@akara
Feb 08 2017 18:48
@anilgursel @sebady @Harikiranvuyyuru PR #379, #381 should be ready to merge. Please give your LGTM if agree.
I have some responses from Codacy.
Need to take action to resolve coverage uploads going forward.
Anil Gursel
@anilgursel
Feb 08 2017 21:11
@akara Here is the PR for DefaultHttpEndpointResolver paypal/squbs#396. I would like to update the doc here https://github.com/paypal/squbs/blob/master/docs/httpclient.md#service-discovery-chain. But, before that I need to find a way to register this resolver by default in squbs-httpclient. We discussed that one way is to make ClientFlow an Akka Extension; however, that would make the API look more different than Akka HTTP client and not sure if it is worth to make it an extension just for this change. Any other alternatives you can think of ?
Anil Gursel
@anilgursel
Feb 08 2017 21:23
@akara any reason why we need to have this file https://github.com/paypal/squbs/blob/master/squbs-httpclient/src/main/resources/logback.xml ? IMO, squbs-httpclient should not bring logback.xml. Unless you have any objections, I will delete it. Also, the commit message suggests that it was checked in when the module was created. Accordingly, I assume it was accidental ?
Can you also please take a look at https://github.com/paypal/squbs/blob/master/squbs-httpclient/src/main/scala/org/squbs/httpclient/pipeline/HttpClientPipeline.scala.waiting. I do not think we need this logic. If anything needed regarding no media type, then we need to do it somewhere in content negotiation logic.
Anil Gursel
@anilgursel
Feb 08 2017 21:32
Regarding DefaultHttpEndpointResolver, I actually take it back. I will convert ClientFlow to an Akka Extension. We anyway pass ActorSystem currently (so it slightly differs from Akka HTTP Client API). Moving ActorSystem to ClientFlow extension style would not make it any worse, might even improve it.
Akara Sucharitakul
@akara
Feb 08 2017 21:36
@anilgursel for logback.xml I think it was there before open source, in error. Please remove.
Anil Gursel
@anilgursel
Feb 08 2017 21:36
:+1:
Akara Sucharitakul
@akara
Feb 08 2017 21:38
Great! I thought the reason we're passing ActorSystem was because it is already an Akka Extension. Looks like I'm wrong. So at least it won't hurt to make it an extension. But lets also keep thinking about alternate solves.
Give me some time with HttpClientPipeline.scala.waiting (it can stay "waiting" a little longer). Couple of things on hand and including fixing the coverage uploads.
Anil Gursel
@anilgursel
Feb 08 2017 21:40

Regarding refactoring ClientFlow to an Akka Extension, the Java API might get little worse though. Looking into it now.. Currently it looks like

ClientFlow.create("sample", system, mat);

I do not want it to look like

ClientFlow.get(system).create("sample", mat);
Akara Sucharitakul
@akara
Feb 08 2017 21:41
That can be easily masked.
Anil Gursel
@anilgursel
Feb 08 2017 21:42
Yeah, that’s what I am thinking; however, was just wondering if it would make the extension not following the conventions.. but, anyway.. let me mask it .. will update the PR soon
Akara Sucharitakul
@akara
Feb 08 2017 21:45
The other way around is to keep a concurrent Set in ClientFlow (it is an object) that holds actor system names that are initialized. If non-existent, add the name and install a new resolver.
Just one other idea. May not be the best one yet.
Anil Gursel
@anilgursel
Feb 08 2017 21:47
I like the Akka Extension better actually. This does not sound simple/clean enough :worried:
Akara Sucharitakul
@akara
Feb 08 2017 21:54
Not much difference. Take a look at ActorSystemImpl and you'll find extensions is just a ConcurrentHashMap[ExtensionId[_]. AnyRef]
Anil Gursel
@anilgursel
Feb 08 2017 22:44
I see
Anil Gursel
@anilgursel
Feb 08 2017 22:51
actually, with the Akka Extension the Scala API would potentially be degrading as well, so using a ConcurrentHashMap might not be too bad..
however, we should also allow unregistering this resolver. With ConcurrentHashMap, we need to add more logic around..
actually not.. ConcurrentHashMap would mark it registration already attempted.. Not as it is in the list or not..
so, should be good
In my previous messages ConcurrentHashMap -> ConcurrentHashSet :smile:
Anil Gursel
@anilgursel
Feb 08 2017 23:53
@akara I had addressed the review comment on paypal/activator-squbs-scala#6 few days back. Please take a look and merge if you get a chance
Akara Sucharitakul
@akara
Feb 08 2017 23:55
Sorry, thought I already took care of that.
Done
Anil Gursel
@anilgursel
Feb 08 2017 23:55
thanks!