feat(extstore): run store and retrieve operations concurrently (default 3, configurable) - #2294
Conversation
…lt 3, configurable) Extstore walks ran every driver call sequentially because the option builders never set a concurrency limit on the payload visit. Store and retrieve calls now run concurrently, capped by the new ExternalStorage.maxConcurrentOperations option (default 3). A limit passed explicitly by a caller still wins. Closes temporalio#2275
| { | ||
| initialTarget, | ||
| deriveContext, | ||
| limit = concurrencyLimit(externalStorage.maxConcurrentOperations), |
There was a problem hiding this comment.
I don't think that's correct. The concurrency set on externalStorage should be shared across all callers, not independently per visitors.
For example, if I set a limit of four on my externalStorage that's because I don't want to ever have more than four pending requests to the store. As it is written now, if I have three tasks being decoded concurrently, each task visitor gets it's own concurrency limit, potentially resulting in a total of 12 concurrent requests.
There was a problem hiding this comment.
Also, I'd argue that if a concurrency limit is set both on the store and on the visitor, then should both apply; i.e. the latter doesn't simply override the former.
There was a problem hiding this comment.
In any case, I'd recommend to wait for @jmaeagle99 to chime in before changing your PR. He's the lead on the external storage initiative, so I'd be interested to hear his opinion. He's out today, but should be back tomorrow.
|
Good point on the sharing semantics, a per-walk limit does multiply under concurrent task decode. Moving the semaphore to the ExternalStorage instance so all walks share one budget, and composing an explicitly passed visitor limit with it instead of overriding both sound right to me. I'll hold off on changes until jmaeagle99 has a chance to weigh in. |
|
I think we've got a few definitions of what a "concurrency limit" means in the context of external storage and should reconcile them.
Also for reference, the Python SDK limits external storage by limiting the payload visiting per activation/message walk and not globally. The limit is applied at the visitor level and not the actual external storage operation level. |
|
Agree these need reconciling. One thing I noticed while sketching the shared version locally: if the semaphore lives on the ExternalStorage instance but is acquired per payload operation (around the actual driver call, not per visit site), it acts as a limit on real storage operations without threading any context into the drivers, since every payload op already funnels through that path. The fan-out case then can't exceed the budget either: one site visiting N payloads takes N acquisitions. That layering would give the instance-level budget the semantics mjameswh described (never more than X in-flight requests to the store), and a per-walk visitor limit could still compose on top for parity with how the Python SDK scopes it. Whether that's the right shape is exactly the jmaeagle99 question, so I'll keep holding off on changes until you all settle direction. Happy to implement whichever way it lands. |
What was changed
External storage store and retrieve calls now run concurrently during a payload walk, capped at 3 by default. The cap is configurable via a new
ExternalStorage.maxConcurrentOperationsoption, validated as a positive integer.Why
The payload visitor already supports a concurrency limit, but the extstore option builders never passed one, so every driver call ran sequentially. Closes #2275.
How
extstoreStoreOptionsandextstoreRetrieveOptionsderive their defaultlimitfromExternalStorage.maxConcurrentOperations, so the cap applies at every store and retrieve site (worker and clients). An explicitlimitstill overrides. The knob lives onExternalStoragerather than worker options so client-side walks get the same behavior and the extstore config stays in one place.Testing
Added unit tests that assert peak driver-call concurrency for store and retrieve walks (3 by default, configurable), plus config validation tests. The extstore unit and integration suites pass against a local dev server.