Readside exponential backoff #329
Conversation
| * @param meta the additional meta data | ||
| * @return Boolean for success | ||
| */ | ||
| protected def doProcessEvent( |
There was a problem hiding this comment.
why is this on the trait? this is an implementation detail
There was a problem hiding this comment.
Wanted to decouple the implementation of "process" with the exponential backoff.
| eventTag: String, | ||
| resultingState: com.google.protobuf.any.Any, | ||
| meta: MetaData, | ||
| policy: Option[retry.Policy] = Some(retry.Directly()), |
There was a problem hiding this comment.
2 comments:
policyshould be a constructor arg, not a method arg- you don't need an
Optionif you fail over toretry.Backoff, just make that the default
There was a problem hiding this comment.
Because the backoff policy itself is a stateful class. I wanted to decouple the max and min time from the default arg
There was a problem hiding this comment.
But yeah I didn't mean to set this as Some(something) whoops.
|
|
||
| implicit val success: retry.Success[Boolean] = retry.Success(x => x) | ||
|
|
||
| val finalPolicy: retry.Policy = |
There was a problem hiding this comment.
this implementation only makes sense for gRPC. Just put it in the impl class.
FYI - this trait was only meant to make testing easier, not really for implementing many of these.
There was a problem hiding this comment.
The policy is not a grpc concept though. It is a retry concept.
code/service/src/main/scala/com/namely/chiefofstate/readside/ReadSideManager.scala
Show resolved
Hide resolved
| import scala.concurrent.ExecutionContext | ||
|
|
||
| trait ExecutionContextHelper { | ||
| implicit val ec: ExecutionContext = ExecutionContext.global |
There was a problem hiding this comment.
I wouldn't do this, just have your test set it explicitly.
There was a problem hiding this comment.
I just felt this was cleaner
|
@AntonioYuen closing this for now |
Builds on the comments from #322