first version of changes to avoid accidental worker state update during worker recovery #408
Conversation
…ng worker recovery
lib/coordinator.go
Outdated
| @@ -831,6 +831,7 @@ func (crd *Coordinator) dispatchRequest(request *netstring.Netstring) error { | |||
| // donot return a stranded worker. recover inserts a good worker back to pool. | |||
| // | |||
| if err == ErrSaturationKill { | |||
| logger.GetLogger().Log(logger.Info, "trigger recovery as part of ErrSaturationKill") | |||
There was a problem hiding this comment.
If the logging is desired, might as well add the worker id.
There was a problem hiding this comment.
Removed this comment
lib/workerclient.go
Outdated
| if worker.Status == status { | ||
| return | ||
| } | ||
| if worker.isUnderRecovery == 1 && !callFromRecovery { |
There was a problem hiding this comment.
If by design a worker's state can only be either init, quce or fnsh, I think it may be feasible and simpler to discard any other state to be set as long as worker.isUnderRecovery is 1 since it is the first thing done after entering workerclient Recover().
| @@ -966,17 +995,23 @@ func (worker *WorkerClient) Write(ns *netstring.Netstring, nsCount uint16) error | |||
|
|
|||
| // setState updates the worker state | |||
| func (worker *WorkerClient) setState(status HeraWorkerStatus) { | |||
| if worker.Status == status { | |||
| currentStatus := worker.Status | |||
There was a problem hiding this comment.
Why is this explictly done..
What happens if worker.Status value changes..?
Shouldn't we use the reflected value in comparison?
There was a problem hiding this comment.
This is to avoid the race-condition on worker.Status value change. Currently we are not locking entire SetState operation.
So there is possibility of worker.Status value can change between worker.Status == status check to Contains(validStateTransitionMap[worker.Status ], status). To avoid it we are using local variable.
No description provided.