Fix fast restart of skygear-server result on worker stale #161
Conversation
Those function belong to broker logic and modify the Broker state that to be used in `Run`
This fix the bug on fast skygear-server restart, with the plugin will not know there is restart and fails to establish the init handshake.
cheungpat
changed the title
| @@ -217,10 +225,11 @@ func (q *workerQueue) Ready(worker pworker) { | |||
| for i, w = range workers { | |||
| if bytes.Equal(w.address, worker.address) { | |||
| *q = append(append(workers[:i], workers[i+1:]...), worker) | |||
There was a problem hiding this comment.
an array that doesn’t work like any other arrays in golang
since we are changing the variable address here... are we sure all references to the array are updated?
There was a problem hiding this comment.
oh you changed it in a later commit! good job!
| switch status { | ||
| case Ready: | ||
| workers.Ready(newWorker(address)) | ||
| lb.freshWorkers <- address |
There was a problem hiding this comment.
looks like a good move to here
| case Heartbeat: | ||
| // do nothing | ||
| // no-ops |
I am not sure if this is the old/undesired or new/desired behaviour |
New, desired behaviour. |
| @@ -194,54 +197,80 @@ func newWorker(address []byte) pworker { | |||
| } | |||
| } | |||
|
|
|||
| type workerQueue []pworker | |||
| type pworkers []pworker | |||
There was a problem hiding this comment.
We could probably get rid of this type alias as it doesn't provide extra value anymore after []pworker is moved as a field of workerQueue
| } | ||
|
|
||
| func (q *workerQueue) Tick(worker pworker) error { | ||
| if _, ok := q.addresses[string(worker.address)]; !ok { |
There was a problem hiding this comment.
You are trying to divide Ready into Add and Tick s.t. a worker which haven't sent any Ready signal couldn't be added via Tick. I think we could further simplify the responsibility of Add and Tick s.t.
Addonly appends a worker to a queue (which is automatically the latest position);Tickonly prompts an existing worker to latest position of the queue, and it is an error toTickan non-existing worker
In this way we can get rid of workerQueue.addresses which is a duplicated state of workerQueue.pworkers.
There was a problem hiding this comment.
workerQueue.pworkers Only holy the idle worker. That is the queue for consuming order. It don't store all previously registered worker. So the state is not duplicated.
Hence, if we make Add only register worker, but not push to the queue. It will take next round heartbeat to push itself to worker.
There was a problem hiding this comment.
Okay I forgot that a worker is removed from the queue before it is handed over a job. It LGTM then.
No I meant Add adds a worker to the queue and Tick will never add a new worker. Anyway it's not relevant now.
There was a problem hiding this comment.
Although the pull request is merged (good job rickmak and limouren!), I’d just like to say that some comments to describe Tick and Add would be a great idea. Maybe next time.
zmq broker don't accept non register plugin heartbeat. It will force the plugin
resend a proper Ready and go through the init process again.
refs #150