search

temporalio / temporal

Temporal service
https://docs.temporal.io
MIT License
11.26k stars 810 forks source link

Make task queues more ordered #2517

Open mfateev opened 2 years ago

mfateev commented 2 years ago

Is your feature request related to a problem? Please describe. Currently, task queue sync match is prioritized over backlog. So a task can sit in a backlog for a long time being starved by new tasks. Ideally task queues should try to deliver tasks in order. It is not possible to guarantee total ordering as multiple partitions and worker processes are involved, but not starving backlogged requests would greatly improve fairness of the system.

Describe the solution you'd like Deliver tasks from backlog first. It can increase DB utilization as all tasks would need to be persisted in the case of a backlog. So we should consider if this should be an option of a task queue. I would try to avoid this and introduce such an option only if we see that DB load increase is significant after performance testing.

paulnpdev commented 2 years ago

I'm concerned about the dynamics of this. I'm not sure if it's true in a realistic sense, but iiuc, when the system is database-throughput-constrained, under loads that the system could handle normally (because of the efficiency of sync-match) we would actually never catch up once we got into a backlog situation where every task has to be both written and read from the DB.

Maybe we could be sophisticated and detect this case? As long as we're making forward progress through time, prioritize backlog; if we are falling behind, prioritize sync-match? A little complicated, yes. But imho worth a little experimental coding to see how complicated. Assuming that I'm understanding the dynamics correctly here.

dnr commented 2 years ago

Sync match isn't prioritized over backlog right now. They both write into one go channel. I suppose the ratio will be tilted toward sync match since there could be multiple sync match writers and only one db writer, but backlog tasks should still get through. We talked about various potential changes to this last year and decided to wait until we had time to address all the concerns more holistically (avoiding starvation, ordering, avoiding increasing load further when load is already elevated).

mfateev commented 1 year ago

I think we should have an option to not perform sync match if there is a backlog.

ShahabT commented 1 year ago

Starting to work on this. As the first step, I'm thinking to add a boolean dynamic config (per namespace+task queue) to allow disabling sync match when there is a backlog. Intention is to manually set this config only when the specific task queue requires FIFOish ordering.

paulnpdev commented 1 year ago

what do you mean by "manually"? Is the intent that a customer can programmatically set this?

On Wed, Sep 6, 2023 at 3:43 PM Shahab Tajik @.***> wrote:

Starting to work on this. As the first step, I'm thinking to add a boolean dynamic config (per namespace+task queue) to allow disabling sync match when there is a backlog. Intention is to manually set this config only when the specific task queue requires FIFOish ordering.

— Reply to this email directly, view it on GitHub https://github.com/temporalio/temporal/issues/2517#issuecomment-1709221757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUMR7H2RBOUJGH2E7XN5NNTXZD4BZANCNFSM5OVPOTSQ . You are receiving this because you commented.Message ID: @.***>

ShahabT commented 1 year ago

@paulnpdev no, this config will not be exposed to the client (at least not now). Only the server operator can turn this flag on. Because this has implications on the load on the persistence layer, I do not think we can allow the customers turning it on, until the dynamics are fully understood as you mentioned.

paulnpdev commented 1 year ago

ok for now! will the flag be at the namespace level, the WF type level, or the TaskQ level

On Thu, Sep 7, 2023 at 11:35 AM Shahab Tajik @.***> wrote:

@paulnpdev https://github.com/paulnpdev no, this config will not be exposed to the client (at least not now). Only the server operator can turn this flag on. Because this has implications on the load on the persistence layer, I do not think we can allow the customers turning it on, until the dynamics are fully understood as you mentioned.

— Reply to this email directly, view it on GitHub https://github.com/temporalio/temporal/issues/2517#issuecomment-1710607198, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUMR7HYYXTQOMAVU2VMOKADXZIHW3ANCNFSM5OVPOTSQ . You are receiving this because you were mentioned.Message ID: @.***>

ShahabT commented 1 year ago

@paulnpdev taskQ level. More specifically, the combination of namespace + taskQ name + taskQ type (workflow vs activity)

ShahabT commented 12 months ago

After deeper analysis based on synthetic and customer workloads, it turned out that:

Good News

For task queues with low/medium load (in terms of tasks per sec) where a single task queue partition can do all the work you can get approximate FIFO ordering by reducing the number of partitions to 1. The following dynamic configs should be used for that:

matching.numTaskqueueWritePartitions:
  - value: 1
    constraints:
      namespace: <namespace name>
      taskQueueName: <task queue name>
matching.numTaskqueueReadPartitions:
  - value: 1
    constraints:
      namespace: <namespace name>
      taskQueueName: <task queue name>

Note that if you are changing this prop for a pre-exsiting task queue, matching.numTaskqueueWritePartitions should be reduced first, and after making sure no unfinished tasks are in the old partitions, you can update matching.numTaskqueueReadPartitions too.

macnibblet commented 1 month ago

@ShahabT How do we do this in the temporal cloud?

paulnpdev commented 1 month ago

Is this something we want to try to fix? It bothers me that we become significantly less efficient when we're processing backlog. Maybe we reserve some of the dispatch capacity for fulfilling sync match and some for processing backlog? Of course it's not that easy, but conceptually?

On Tue, Sep 12, 2023 at 10:36 AM Shahab Tajik @.***> wrote:

After deeper analysis based on synthetic and customer workloads, it turned out that:

  • Sync match rarely happens in presence of a backlog. This is because sync match request (for tasks coming directly from history) only go through if a poller is available in that very moment, they do not wait and compete with the pending backlog task for new pollers to become available.
  • Task queue partitioning is the main reason for dispatch order mixups. Due to the asymmetricity of our root vs leaf partitions, when there is a backlog, the root partition tends to process it's backlog much faster and the leaf partitions accumulate heavier/older backlogs. [I'm investigating how/why this happens]

Good News

For task queues with low/medium load (in terms of tasks per sec) where a single task queue partition can do all the work you can get approximate FIFO ordering by reducing the number of partitions to 1. The following dynamic configs should be used for that:

matching.numTaskqueueWritePartitions:

  • value: 1 constraints: namespace: taskQueueName: matching.numTaskqueueReadPartitions:
  • value: 1 constraints: namespace: taskQueueName:

Note that if you are changing this prop for a pre-exsiting task queue, matching.numTaskqueueWritePartitions should be reduced first, and after making sure no unfinished tasks are in the old partitions, you can update matching.numTaskqueueReadPartitions too.

— Reply to this email directly, view it on GitHub https://github.com/temporalio/temporal/issues/2517#issuecomment-1716153786, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUMR7H244NADJG2RISXT5PTX2CMQDANCNFSM5OVPOTSQ . You are receiving this because you were mentioned.Message ID: @.***>