-
Notifications
You must be signed in to change notification settings - Fork 168
Update storage driver store context metadata #1399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 12 commits
5a19712
8f14f6b
50ffea2
f7ed149
700b73c
31d6509
9c392c8
d80338b
3d84175
ff30607
77ae64d
b57c6ec
a6c5ca2
e2881d0
98677f6
019da3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,6 @@ | |
|
|
||
| from temporalio.api.common.v1 import Payload, Payloads | ||
| from temporalio.converter._payload_converter import JSONPlainPayloadConverter | ||
| from temporalio.converter._serialization_context import ( | ||
| SerializationContext, | ||
| WithSerializationContext, | ||
| ) | ||
|
|
||
| _T = TypeVar("_T") | ||
|
|
||
|
|
@@ -92,6 +88,83 @@ class StorageDriverClaim: | |
| """ | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class StorageDriverWorkflowInfo: | ||
| """Workflow identity information for external storage operations. | ||
|
|
||
| .. warning:: | ||
| This API is experimental. | ||
| """ | ||
|
|
||
| namespace: str | ||
| """The namespace of the workflow execution.""" | ||
|
|
||
| id: str | None = None | ||
| """The workflow ID.""" | ||
|
|
||
| run_id: str | None = None | ||
| """The workflow run ID, if available.""" | ||
|
|
||
| type: str | None = None | ||
| """The workflow type name, if available.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class StorageDriverActivityInfo: | ||
| """Activity identity information for external storage operations. | ||
|
|
||
| .. warning:: | ||
| This API is experimental. | ||
| """ | ||
|
|
||
| namespace: str | ||
| """The namespace of the activity execution.""" | ||
|
|
||
| id: str | None = None | ||
| """The activity ID.""" | ||
|
|
||
| run_id: str | None = None | ||
| """The activity run ID (only for standalone activities).""" | ||
|
|
||
| type: str | None = None | ||
| """The activity type name, if available.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class StorageDriverStoreMetadata: | ||
| """Store-only metadata available during external storage operations. | ||
|
|
||
| .. warning:: | ||
| This API is experimental. | ||
| """ | ||
|
|
||
| target: StorageDriverActivityInfo | StorageDriverWorkflowInfo | None = None | ||
| """The workflow or activity for which this payload is being stored.""" | ||
|
|
||
|
|
||
| _current_store_metadata: contextvars.ContextVar[StorageDriverStoreMetadata | None] = ( | ||
| contextvars.ContextVar("_current_store_metadata", default=None) | ||
| ) | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def store_metadata_context( | ||
| metadata: StorageDriverStoreMetadata | None, | ||
| ) -> Generator[None, None, None]: | ||
| """Context manager that sets store metadata and resets it on exit. | ||
|
|
||
| If metadata is None, yields without setting anything. | ||
| """ | ||
| if metadata is None: | ||
| yield | ||
| return | ||
| token = _current_store_metadata.set(metadata) | ||
| try: | ||
| yield | ||
| finally: | ||
| _current_store_metadata.reset(token) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class StorageDriverStoreContext: | ||
| """Context passed to :meth:`StorageDriver.store` and ``driver_selector`` calls. | ||
|
|
@@ -100,10 +173,18 @@ class StorageDriverStoreContext: | |
| This API is experimental. | ||
| """ | ||
|
|
||
| serialization_context: SerializationContext | None = None | ||
| """The serialization context active when this store operation was initiated, | ||
| or ``None`` if no context has been set. | ||
| """ | ||
| target: StorageDriverActivityInfo | StorageDriverWorkflowInfo | None = None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "target" struck me as odd since it's an info rather than a target. then my second thought was that target would be "where it's stored" rather than "what's being called."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like you said, Implementation wise, the Maybe:
|
||
| """The workflow or activity for which this payload is being stored. | ||
|
|
||
| For payloads being stored on behalf of an explicit target (e.g. a child | ||
| workflow being started, an activity being scheduled, an external workflow | ||
| being signaled), this is that target's identity. When no explicit target | ||
| exists the current execution context (workflow or activity) is used as the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. separate
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. During code review, we talked about the idea of providing both "caller" and "callee" information and felt that it was ambiguous as to how a driver author was supposed to use them, because you don't always store in the callee context. When talking in terms of the S3 driver, there was an overall pattern of we always want to store the information in a key space that best suits itself for replay and for lifecycle management. That mostly meant the "target workflow" (e.g. child workflow, external signal, CaN, completing workflow that does have a parent) but is sometimes the the current workflow" (e.g. workflow activities, completing workflow that doesn't have a parent); sometimes it is the "current activity" (completing standalone activity) but is sometimes the "target activity" (client starts a standalone activity). Just providing "caller" and "callee" information isn't enough; you also need to know standalone vs workflow activity. Maybe that's all the extra was needed but the algorithm is not easy to implement. We felt that this determination would be the same across drivers, if they cared to store payloads in a contextual hierarchy. So it was built into the external storage layer rather than just a concern driver authors had to think about. |
||
| target instead. | ||
|
|
||
| The :attr:`StorageDriverWorkflowInfo.namespace` or | ||
jmaeagle99 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| :attr:`StorageDriverActivityInfo.namespace` field on the target carries the | ||
| namespace for the execution, when available.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
|
|
@@ -182,7 +263,7 @@ class _StorageReference: | |
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ExternalStorage(WithSerializationContext): | ||
| class ExternalStorage: | ||
| """Configuration for external storage behavior. | ||
|
|
||
| .. warning:: | ||
|
|
@@ -223,10 +304,6 @@ class ExternalStorage(WithSerializationContext): | |
| for retrieval lookups. | ||
| """ | ||
|
|
||
| _context: SerializationContext | None = dataclasses.field( | ||
| init=False, default=None, repr=False, compare=False | ||
| ) | ||
|
|
||
| _claim_converter: ClassVar[JSONPlainPayloadConverter] = JSONPlainPayloadConverter( | ||
| encoding=_REFERENCE_ENCODING.decode() | ||
| ) | ||
|
|
@@ -257,12 +334,6 @@ def __post_init__(self) -> None: | |
| driver_map[name] = driver | ||
| object.__setattr__(self, "_driver_map", driver_map) | ||
|
|
||
| def with_context(self, context: SerializationContext) -> Self: | ||
| """Return a copy of these options with the serialization context applied.""" | ||
| result = dataclasses.replace(self) | ||
| object.__setattr__(result, "_context", context) | ||
| return result | ||
|
|
||
| def _select_driver( | ||
| self, context: StorageDriverStoreContext, payload: Payload | ||
| ) -> StorageDriver | None: | ||
|
|
@@ -292,9 +363,16 @@ def _get_driver_by_name(self, name: str) -> StorageDriver: | |
| raise ValueError(f"No driver found with name '{name}'") | ||
| return driver | ||
|
|
||
| @staticmethod | ||
| def _build_store_context() -> StorageDriverStoreContext: | ||
| meta = _current_store_metadata.get() | ||
| return StorageDriverStoreContext( | ||
| target=meta.target if meta else None, | ||
| ) | ||
|
|
||
| async def _store_payload(self, payload: Payload) -> Payload: | ||
| start_time = time.monotonic() | ||
| context = StorageDriverStoreContext(serialization_context=self._context) | ||
| context = self._build_store_context() | ||
|
|
||
| driver = self._select_driver(context, payload) | ||
| if driver is None: | ||
|
|
@@ -335,7 +413,7 @@ async def _store_payload_sequence( | |
| start_time = time.monotonic() | ||
|
|
||
| results = list(payloads) | ||
| context = StorageDriverStoreContext(serialization_context=self._context) | ||
| context = self._build_store_context() | ||
|
|
||
| to_store: list[tuple[int, Payload, StorageDriver]] = [] | ||
| for index, payload in enumerate(payloads): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.