More efficient generation of ImmutableWorkerHolder from WorkerHolder.#14546
More efficient generation of ImmutableWorkerHolder from WorkerHolder.#14546gianm merged 8 commits intoapache:masterfrom
Conversation
Taking the work done in apache#12096 a little further: 1) Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner). The original patch only helped with the ZkWorker (RemoteTaskRunner). 2) Improve the ZkWorker version somewhat by avoiding multiple iterations through the task announcements map.
| for (final Map.Entry<String, TaskAnnouncement> entry : announcements.entrySet()) { | ||
| final TaskAnnouncement announcement = entry.getValue(); | ||
|
|
||
| if (announcement.getStatus().isRunnable()) { |
There was a problem hiding this comment.
Query: WorkerHolder was already doing this filtering but ZkWorkerHolder seems to be using all the announcements (unless the filtering happens there at some prior stage). Was it performing incorrect computations of capacity and the other parameters?
There was a problem hiding this comment.
Hmm, I didn't notice this behavioral difference til now. I think the filtering on isRunnable is the correct thing to do: tasks don't use slots when they aren't running, so they shouldn't be counted.
With this change, some RemoteTaskRunnerTest cases need to be adjusted, but I think that's OK. Note that in most cases, tasks get removed from the announcement set pretty quickly after they complete, so I don't expect a big practical difference in prod.
There was a problem hiding this comment.
Yeah, filtering here makes sense.
…apache#14546) * More efficient generation of ImmutableWorkerHolder from WorkerHolder. Taking the work done in apache#12096 a little further: 1) Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner). The original patch only helped with the ZkWorker (RemoteTaskRunner). 2) Improve the ZkWorker version somewhat by avoiding multiple iterations through the task announcements map. * Pick better names and use better logic. * Only runnable tasks. * Fix test. * Fix testBlacklistZKWorkers50Percent.
3 participants
Taking the work done in #12096 a little further:
Applying a similar optimization to WorkerHolder (HttpRemoteTaskRunner).
The original patch only helped with the ZkWorker (RemoteTaskRunner).
Improve the ZkWorker version somewhat by avoiding multiple iterations
through the task announcements map.