Implement planned topic: 0011-external-storage - #209
Merged
Conversation
![skill-sync[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Python: - Import ClientConfig from temporalio.envconfig, not temporalio.client. load_client_connect_config() is a staticmethod on the envconfig class; the temporalio.client.ClientConfig TypedDict has no such member, so the snippet raised AttributeError. Follow main's env-config convention (setdefault target_host) from #261. - Register real Workflow/Activity placeholders. Worker() with empty workflows and activities raises "At least one activity, Nexus service, or workflow must be specified", and wrap the setup in async main(). Go: - Cover the GCS driver (contrib/gcp/gcsdriver + gcssdk), which the SDK ships and the docs install alongside S3. - Load client options with envconfig.MustLoadDefaultClientOptions() and note that Workers inherit External Storage from their Client. Align coverage across all three languages, each of which was missing something the others had: - 50 MiB MaxPayloadSize/max_payload_size ceiling and the matching anti-pattern (Go, Python). - Store/Retrieve are not retried within a Task attempt; the Task retries as a whole, so storage must be idempotent (Go, Python). - Multi-region durability with CRR + an MRAP ARN (Go, Python). - Distinct driver names when registering two drivers of the same kind (Go, Python). - Codec Server guidance (TypeScript), including that neither the TypeScript nor Python SDK ships a storage-aware handler. - Built-in driver behavior sections (concurrency, content-addressed keys, integrity checks, diagnostics) in Go and Python. - ctx.Context on the Go driver contexts, mirroring TypeScript's abortSignal guidance; optional type() override in Python. Also: standardize the TypeScript Public Preview admonition on the repo's wording, drop the transplanted `payloadSizeThreshold: 1` anti-pattern (TypeScript compares >=, so 1 behaves like 0), replace site-relative plugins-guide links with absolute URLs, refresh the index pointers, and revert an unrelated whitespace change in the Spring AI reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address code-review findings on the new external storage docs: - Go: add missing "context" and "log" imports to the S3 driver, GCS driver, and client/worker setup snippets, which presented complete import lists but failed to compile. - Go: add go.temporal.io/sdk/contrib/envconfig to both go get lines; it is a separate module and is imported by the setup snippet. - Go: give the local-disk worked example an import block, and introduce the commonpb alias at its first use in the selector example. - Go and Python: validate claim data in Retrieve/retrieve so a hand-crafted reference payload cannot read files outside the store directory, matching the hardening already applied to Store/store. - Python: the Worker inherits the Data Converter from its Client and takes no data_converter argument; the prose said to pass it to both. Verified by compiling every Go snippet against sdk-go and exercising both path guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The new external storage docs were only reachable from the language index files, so the paths an agent actually takes when a user hits a payload limit still sent it to hand-roll the claim-check pattern. - core/error-reference.md: TMPRL1103 recovery now points at built-in External Storage before manual reference passing. - core/gotchas.md: the payload-limit fix notes the SDK does this for you in Go, Python, and TypeScript. - core/patterns.md: Large Data Handling leads with the SDK-native option and scopes the manual pattern to the cases that need it. Also link the Go external storage sample from the Codec Server section, matching what the Python reference already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Validation Report —
external-storageScope: the two new reference files introduced on
draft/0011-external-storage:references/go/external-storage.mdreferences/python/external-storage.mdplus the one-line pointers added to
references/go/go.mdandreferences/python/python.md.Source of truth:
../documentation/docs/. Primary subtrees consulted:docs/encyclopedia/data-conversion/external-storage.mdxdocs/encyclopedia/data-conversion/codec-server.mdxdocs/develop/go/best-practices/data-handling/external-storage.mdxdocs/develop/python/best-practices/data-handling/external-storage.mdxIntegration topic? No — External Storage is a built-in SDK feature, not a third-party integration. Check 5 (integration-layout audit) does not apply.
Go/no-go
Overall verdict: GO.
The two Python import-path misses (Check 2) fall under the MINOR FIXES bucket per §5: ≤ 5 unexplained misses that look like missing-citation-comment cases. They do not block merge. Optional follow-up: tag the two
from temporalio.contrib.aioboto3/from temporalio.external_storagelines with<!-- undocumented: source = inferred from docs symbols + SDK module convention -->or replace them with the exact import lines used by the upstream samples-python repo.Check 1 findings — citation audit
Zero unresolved citations.
<!-- docs/...:N-M -->resolves to a file under../documentation/docs/, the line range exists, and the preceding authored claim is substantively supported by the cited text.Spot-checked highlights that survived independent re-reading:
encyclopedia/external-storage.mdx:42-45.encyclopedia/external-storage.mdx:72-73,98-99.PayloadSizeThreshold: 1externalizes all,0is interpreted as the default (256 KiB) —develop/go/.../external-storage.mdx:232-233.payload_size_threshold=0externalizes all —develop/python/.../external-storage.mdx:196-197.StorageDriverActivityInfois for standalone (non-workflow-bound) Activities only —develop/go/.../external-storage.mdx:128-130, 204-208./download,/decode,/encode,?preserveStorageRefs=trueendpoints —encyclopedia/codec-server.mdx:105-114.NewPayloadHTTPHandlervsNewPayloadCodecHTTPHandler—encyclopedia/codec-server.mdx:96-103.Check 2 findings — reverse-grep audit
Go file: zero unexplained misses.
All 21 Temporal Go API symbols (
s3driver.NewDriver,s3driver.StaticBucket,s3driver.Options,awssdkv2.NewClient,client.Dial,client.Options,converter.ExternalStorage,converter.StorageDriver,StorageDriverClaim,StorageDriverStoreContext,StorageDriverRetrieveContext,StorageDriverWorkflowInfo,StorageDriverActivityInfo,StorageDriverSelector,DriverSelector,PayloadSizeThreshold,Drivers,Name(),Type(),Store(...),Retrieve(...),NewPayloadHTTPHandler,PayloadHTTPHandlerOptions,NewPayloadCodecHTTPHandler), all import paths (go.temporal.io/sdk/contrib/aws/s3driver,.../s3driver/awssdkv2,go.temporal.io/sdk/converter), all four endpoint strings, and all threshold sentinels appear in the source-of-truth docs.Python file: 2 unexplained misses.
temporalio.contrib.aioboto3from temporalio.contrib.aioboto3 import new_aioboto3_client)../documentation/docs/. The symbolnew_aioboto3_clientis documented (develop/python/.../external-storage.mdx:50) but its import line is hidden inside an external SNIPSTART file (features/snippets/external_storage/s3_setup/s3_driver_create.py) that is not part of the docs tree. Author inferred the path from the SDK'stemporalio.contrib.*convention. Plausible but not directly verifiable against docs.temporalio.external_storagefrom temporalio.external_storage import ...)ExternalStorage,S3StorageDriver,StorageDriver,StorageDriverClaim,StorageDriverStoreContext,StorageDriverRetrieveContext,StorageDriverWorkflowInfo) are all documented, but their import path is not.Discussed but not a finding:
payload_size_threshold=1(Python anti-patterns, line 229) — only appears in the negative bullet warning against using Go's sentinel. The cross-language statement "where0is the default and1externalizes all" is anchored in the Go docs (develop/go/.../external-storage.mdx:232-233). Valid cross-reference.<!-- docs/...:24-30 documents Pre-Release status -->comment, so the mapping is intentional, not fabrication.Verdict: 2 misses fall under MINOR FIXES (≤ 5, missing-citation-comment style). Recommended fix is a one-line tag like
<!-- undocumented: inferred from temporalio.contrib.* convention; cross-check against samples-python -->on the import block, or substitution of the verified imports from the upstream SNIPSTART files.Check 3 findings — regression patterns
Zero hits. None of the universal regression patterns (
--profile, the three legacy TLS env-var names,tcld service-account,--output text/jsonl,saas-api.tmprl.cloud:7233) appear anywhere in either authored file.Topic-specific regression sentinels were checked:
PayloadSizeThreshold: 0in the Go file appears only inside anti-pattern bullets explicitly warning against that misuse (lines 93 and 248) — correct guidance, not a regression.payload_size_threshold=1in the Python file appears only inside the anti-pattern bullet explicitly warning against that misuse (line 229) — correct guidance, not a regression.The skill correctly captures the Go/Python sentinel asymmetry (Go:
1externalizes all,0is default; Python:0externalizes all) and calls out the cross-language footgun.Check 4 findings — independent re-verification (sampling)
Go file — 10/10 match.
Sampled authored lines (with cited docs): L6 (encyclopedia:24-30), L14 (encyclopedia:43-45), L21 (encyclopedia:72-73,98-99), L30 (encyclopedia:90-92), L85 (develop/go:41-42), L92 (develop/go:232-233), L107 (develop/go:249-252), L134 (develop/go:185-195), L143 (develop/go:123-134,204-208), L235 (encyclopedia:131-143).
All ten authored claims were re-formed independently from the docs and matched. Notable nuances that survived re-reading: 2 MB Cloud-fixed vs self-hosted-configurable, four-method custom driver,
StorageDriverActivityInfostandalone-only, multi-driver retrieval semantics, parallel within a single Workflow Task.Python file — 10/10 match.
Sampled authored lines: L6 (encyclopedia:24-30), L15 (encyclopedia:47-49,55-56), L21 (encyclopedia:72-73,98-99), L30 (encyclopedia:90-92), L76 (develop/python:83-84), L113 (develop/python:148-156), L123 (develop/python:95-144), L200 (codec-server:82-89), L212 (codec-server:200), L229 (develop/python:196-197).
All ten matched. The three-vs-four method count difference between Python (
name,store,retrieve) and Go (Name,Type,Store,Retrieve) is correctly preserved per SDK.Check 6 findings — tone and scope
Zero findings.
if you really|as a last resort|if you must|undocumented way|you can technically|workaround|hack— no hits in either external-storage file. Anti-pattern bullets follow the supported "Don't do X. Use Y instead." form throughout.> [!NOTE]admonition with the canonical phrasing at line 4. The terminological gap between docs ("Pre-Release") and skill ("Public Preview") is acknowledged inline via the immediately following<!-- docs/...:24-30 documents Pre-Release status -->comment.Statistics
Verdict
GO — merge as-is.
Optional follow-up (not blocking): add an
<!-- undocumented: ... -->tag on the two Python import statements (from temporalio.contrib.aioboto3 import new_aioboto3_clientand thefrom temporalio.external_storage import ...blocks at lines 48 and 131) so the import paths are explicitly marked as inferred from the SDK module convention rather than from the docs prose. Alternatively, copy the verified imports from the upstream samples-python files referenced by the SNIPSTART markers indevelop/python/.../external-storage.mdx.