enable grpc retry #322
enable grpc retry #322
Conversation
AntonioYuen
requested review from
klvmungai,
Tochemey and
zenyui
as code owners
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
+ Coverage 83.77% 84.04% +0.26%
==========================================
Files 46 46
Lines 900 915 +15
Branches 19 19
==========================================
+ Hits 754 769 +15
Misses 146 146
Continue to review full report at Codecov.
|
| if (!isSuccess && (maxAttempts <= 0 || numAttempts >= maxAttempts)) { | ||
| val backoffSeconds: Long = Math.min(maxBackoffSeconds, (minBackoffSeconds * Math.pow(1.1, numAttempts)).toLong) | ||
|
|
||
| Thread.sleep(Duration.ofSeconds(backoffSeconds).toMillis) |
There was a problem hiding this comment.
This is a bit dangerous because we are inside an actor. The projection is an actor making the grpc call. Let us just becareful here
There was a problem hiding this comment.
What do you suggest in this scenario?
There was a problem hiding this comment.
I think we should make the grpc call asynchronous and make use scala retry on future. That will make it more resilient.
For instance you can use these libraries:
- https://github.com/softwaremill/retry (recommended because of the implementation is based upon this algo https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/)
- https://doc.akka.io/docs/akka/current/futures.html#retry: the downside with this is you need the actor system and I don't think it is necessary.
This is my opinion I don't see why you should reimplement something that it is already out there in form of libraries in the jvm ecosystem.
There was a problem hiding this comment.
So instead of using the blocking stub of the grpc client we can use the non-blocking one.
There was a problem hiding this comment.
@zenyui we need to take into consideration this:
for the projection to listen and read events persisted the sharded daemon is hooked it into the existing actor system. With that type of setup I have some worries around the execution context. Since the actor system is making use of the default dispatcher the retry mechanism can hinder a bit the performance of the system(writeside). I think we may have to push this feature into its own execution context to avoid hindering the whole actor system.
There was a problem hiding this comment.
@Tochemey I'm fine with using the retry package, however it's using promises, which is essentially blocking an entire thread just like a Thread.sleep. Why would this be superior aside from their jitter implementation?
There was a problem hiding this comment.
Funny thing is... I started with the amazon algo, then was asked to simplify it :D
There was a problem hiding this comment.
That is why I am saying we may have to consider a separate thread executor. Whatever we do needs another thread executor to think of it properly. These libraries are just suggestions.
There was a problem hiding this comment.
The projection JDBC handler we are using does not have its own its thread pool compared to the slick version we stopped using.
There was a problem hiding this comment.
@Tochemey the call is already blocking, I don't see the issue. I don't mind creating a new thread pool for remote calls, but using a future won't change anything. Under the hood, they are likely just using sleep.
code/service/src/test/scala/com/namely/chiefofstate/readside/Cancellable.scala
Show resolved
Hide resolved
Enables retry for our readside grpc services.