spinach icon indicating copy to clipboard operation
spinach copied to clipboard

Restarting the engine unconditionally resets the max_concurrency values

Open 0xDEC0DE opened this issue 2 years ago • 2 comments

This one is a bit complicated, but:

Preamble

A test case script:

import time
import spinach
sp = spinach.Engine(spinach.RedisBroker())

@sp.task(name='nap', max_retries=1, max_concurrency=8)
def nap(job_id):
    print(f"{job_id:3} Zzzz...")
    time.sleep(5)

batch = spinach.Batch()
for i in range(32):
    batch.schedule('nap', i)

sp.schedule_batch(batch)
sp.start_workers(32, stop_when_queue_empty=True)

Steps to reproduce

  • docker-compose -f spinach/tests/docker-compose.yml up -d
  • Run the script above, it should process the queue 8-at-a-time
  • Run redis-cli and execute the command HSET spinach/_max_concurrency nap 32
  • Run the script again

Expected result

The second run of the script runs all 32 tasks simultaneously, like we told it to

Actual behavior

The second run of the script processes the queue 8-at-a-time

Miscellany

  • This is not a bug; it is 100% intentional behavior, with comments in the set_concurrency_keys.lua script explaining that it is done on purpose.
  • Our app has several Spinach tasks with max_concurrency set to "baseline" values, and Operations occasionally has a need to tune the values up or down, for reasons.
  • This is highly inconvenient for things like flask_spinach running under Gunicorn; when Gunicorn cycles out workers, the new workers start and destroy any runtime adjustments to the Redis that operators may have made; and it does so with no warning.
  • Conversely, it is highly convenient if an Operator wants to, e.g., set a "burst" value for a Spinach task, and immediately forget about it; the software will put itself back on its own.
  • The previous behavior can be simulated by putting a TTL on the _max_concurrency key, but Issue #15 makes it not work very well.
  • Swinging the concurrency adjustments through the app, rather than fidgeting with Redis underneath it, is also an obvious dodge to this issue.

Any thoughts and insights on how to best address this would be appreciated.

0xDEC0DE avatar Nov 28 '23 01:11 0xDEC0DE

This is not a bug; it is 100% intentional behavior, with comments in the set_concurrency_keys.lua script explaining that it is done on purpose.

This is the only place that sets up the key, so naturally it's going to override the current value if one was set from a different client.

I'd advocate for a second key that allows user to set their own limits, effectively as an override for the code's own limit. We can amend the Lua script so it looks for that key first and falls back to the real one. Users could potentially add a TTL on the second key to get the "temporary behaviour" action.

bigjools avatar Nov 28 '23 01:11 bigjools

In fact I'd say let's add a tool script to do this, so that the user doesn't need to know about our Redis keys. Something like: spinach max_concurrency myjobname 32 1m to set myjobname job to 32 concurrency for 1 minute.

bigjools avatar Nov 28 '23 01:11 bigjools