-
Notifications
You must be signed in to change notification settings - Fork 441
Port distributed workers to use reliable delivery work pulling #186
Port distributed workers to use reliable delivery work pulling #186
Conversation
akka-sample-distributed-workers-scala/src/main/scala/worker/Main.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great to see this in action
akka-sample-distributed-workers-scala/src/main/scala/worker/Main.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Outdated
Show resolved
Hide resolved
@@ -35,14 +32,12 @@ object Worker { | |||
workExecutorFactory: () => Behavior[ExecuteWork] = () => WorkExecutor()): Behavior[Message] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workManagerProxy
can be removed, not used
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Outdated
Show resolved
Hide resolved
|
||
// the set of available workers is not event sourced as it depends on the current set of workers | ||
var workers = Map[ActorRef[WorkerCommand], WorkerState]() | ||
var requestNext = Queue[RequestNext[WorkerCommand]]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand why this would have to be a Queue? isn't it enough with Option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my misunderstanding of how the fan out producer worked
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Show resolved
Hide resolved
|
||
def notifyWorkers(workState: WorkState): Unit = | ||
def tryStartWork(workState: WorkState): Effect[WorkDomainEvent, WorkState] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be something for RecoveryCompleted
that would resend the pendingWork
to the WorkPullingProducerController?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This will have to wait for demand from the WPPC. I wonder if we should allow something similar to sharding where we can send more than one message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's convenient with the buffering, but also easy to abuse a loose the flow control. For sharding there is not much choice because it must be possible to send to a new entityId (that hasn't been started yet). I think we shall wait and see if we get any more feedback about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the workInProgress that we need to resent, the pendingWork hasn't been sent yet and will when demand is received. We can have an event that puts work in progress back into the pending queue
I'll pick this back up now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking very good, just a few small things and then this can be merged
|
||
def notifyWorkers(workState: WorkState): Unit = | ||
def tryStartWork(workState: WorkState): Effect[WorkDomainEvent, WorkState] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's convenient with the buffering, but also easy to abuse a loose the flow control. For sharding there is not much choice because it must be possible to send to a new entityId (that hasn't been started yet). I think we shall wait and see if we get any more feedback about it.
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/Worker.scala
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Outdated
Show resolved
Hide resolved
akka-sample-distributed-workers-scala/src/main/scala/worker/WorkManager.scala
Outdated
Show resolved
Hide resolved
@chbatey for next gardening days it would be great to complete this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we forgot this again, but now better to update to 2.6.6 and enable sbr before merging
i'll do that now |
…ithub.com:akka/akka-samples into wip-chbatey-reliable-delivery-distributed-workers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Local push of the reliable delivery PR