configurable execution result store #29
Conversation
| execution_result_store = bootstrap_execution_result_store(config=config) | ||
|
|
||
| storage_backend_type = ExecutionResultStorageBackendType( | ||
| config.get_str('OSPREY_EXECUTION_RESULT_STORAGE_BACKEND', 'none') |
There was a problem hiding this comment.
should none be the default, or should it be minio like the current behavior would result in?
There was a problem hiding this comment.
i think none, because minio is extra setup cost;
with that being said, i think we can have minio setup in the example plugins (?) we talked about moving it there during our sync today
There was a problem hiding this comment.
ya for now i left it here
seems like a fine thing tho to move to examples, i can clean that up if ppl have opinions on what to do with it
| from enum import StrEnum, auto | ||
|
|
||
|
|
||
| class ExecutionResultStorageBackendType(StrEnum): |
There was a problem hiding this comment.
afaict, the ones i added here are the only ones that exist
| if storage_backend is None: | ||
| raise AssertionError('No storage backend registered') | ||
|
|
There was a problem hiding this comment.
afaict, it makes sense to raise here. the codepaths i see calling bootstrap_execution_result_storage_service are all in the ui api or in one of the cli tools, and they probably should be raising an error when there isn't a configured backend.
haileyok
requested review from
a team,
BinaryFiddler,
EXBreder,
ayubun and
jaredmiller13
as code owners
|
atp i only tested that this works with "none" as an option, i'll run with minio in a bit to make sure that still works |
osprey_worker/src/osprey/worker/lib/storage/stored_execution_result.py
Outdated
Show resolved
Hide resolved
| execution_result_store = bootstrap_execution_result_store(config=config) | ||
|
|
||
| storage_backend_type = ExecutionResultStorageBackendType( | ||
| config.get_str('OSPREY_EXECUTION_RESULT_STORAGE_BACKEND', 'none') |
There was a problem hiding this comment.
i think none, because minio is extra setup cost;
with that being said, i think we can have minio setup in the example plugins (?) we talked about moving it there during our sync today
Co-authored-by: ayu <ayu@ayu.dev>
Co-authored-by: ayu <ayu@ayu.dev>
|
hi @haileyok ! Is this PR ready for review? |
|
@julietshen it used to be, though I'm not sure what all has changed since I opened the PR...i do think its still needed though if you're not using minio iirc (i use minio for my own instance so haven't tried recently). i fixed merge conflicts i think |
right now, there isn't a good way to configure or disable the execution result store. the only way i found was to use
register_execution_result_store()inregister_pluginsbut explicitly create and return a noopExecutionResultStore. there's a misleading env var right now in thedocker-compose.ymlcalledOSPREY_EXECUTION_RESULT_STORAGE_BACKENDthat doesn't seem to do anything as well. this results in the worker failing to process events as the requests to the minio store simply fail.here, i took the similar route that exists for configuring the input stream
ExecutionResultStorageBackendTypeenum with Bigtable, GCS, Minio, or "plugin" as options, as well as a simple "None" optionbootstrap_execution_result_store