feat: add payload offloader support (Approach B) - #649
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| /** Serializes and optionally offloads a value. */ | ||
| public String serialize(Object value, SerDes serDes, PayloadOffloader offloader, PayloadOffloadContext context) { | ||
| var serialized = serDes.serialize(value); |
There was a problem hiding this comment.
Codex AI review
[P1] JacksonSerDes.serialize(null) returns null, so an active filesystem offloader rejects valid null results; a permissive offloader would still fail when null is inserted into ConcurrentHashMap. This breaks nullable root/step/child results and the internal Void step used by waitForCallback. Bypass offloading and caching for null serialized values and add null-result replay coverage.
| var serialized = serDes.serialize(value); | |
| var serialized = serDes.serialize(value); | |
| if (serialized == null) { | |
| return null; | |
| } |
| .tenantId(invokeConfig.tenantId()) | ||
| .build()) | ||
| .payload(payloadSerDes.serialize(this.payload)); | ||
| .payload(serializePayload(this.payload, payloadSerDes, SerDesPayloadKind.INVOKE_PAYLOAD, null)); |
There was a problem hiding this comment.
Codex AI review
[P1] OperationUpdate.payload is sent directly to the invoked Lambda. With any active offloader, including OVERFLOW returning inline data, this replaces the target's JSON input with an @aws-durable-payload envelope. Standard Lambda targets cannot decode it, and durable targets require matching storage configuration. Keep chained-invoke wire payloads serialized normally, or make this an explicit target-aware opt-in with corresponding decode support and integration tests.
|
|
||
| <modules> | ||
| <module>sdk</module> | ||
| <module>extra-filesystem-offloader</module> |
There was a problem hiding this comment.
Codex AI review
[P1] Adding the module to the reactor does not publish it: .github/scripts/maven_publish.sh deploys only sdk, sdk-testing, and otel-plugin. Consequently the documented filesystem artifact will never reach Maven Central. Add extra-filesystem-offloader to the deploy script and release artifact upload/verification workflow.
| return executionManager | ||
| .getPayloadCodec() | ||
| .serialize(value, serDes, payloadOffloader, payloadContext(payloadKind, attempt)); |
There was a problem hiding this comment.
Codex AI review
[P2] Offloading occurs before callers decide whether a payload will be checkpointed. Flat/virtual map branches, branches finishing after their parent, and completed parallel replays therefore write unreferenced files; replay can also overwrite a stable filesystem reference despite skipping the update. Perform offloading only for updates that will actually be persisted, while using plain SerDes normalization for virtual/skipped/replayed results. Add flat-map and completed-parallel replay tests using an offload call counter.
| if (storageMode == PayloadOffloadMode.OVERFLOW | ||
| && serializedPayload.getBytes(StandardCharsets.UTF_8).length <= OVERFLOW_THRESHOLD_BYTES) { | ||
| return OffloadedPayload.inline(serializedPayload); |
There was a problem hiding this comment.
Codex AI review
[P2] OVERFLOW checks the serialized text size before that text is JSON-escaped inside the SDK envelope. Payloads containing many quotes or backslashes can be below 255 KB here but exceed 256 KB after envelope encoding, causing checkpoint rejection instead of offloading. Base the decision on the final UTF-8 envelope size, or conservatively account for JSON escaping, and test highly escapable payloads near the threshold.
Codex AI reviewFive confirmed issues block reliable payload offloading, including compatibility and publication failures. Reviewed commit |
| var serialized = serDes.serialize(value); | ||
| var effectiveOffloader = effectiveOffloader(offloader); | ||
| if (effectiveOffloader == null) { | ||
| return serialized; | ||
| } |
There was a problem hiding this comment.
Claude AI review
Null payloads throw when an offloader is configured. serDes.serialize(value) returns null for a null value (see JacksonSerDes.serialize), and this then calls effectiveOffloader.offload(null, context). FileSystemPayloadOffloader.offload starts with Objects.requireNonNull(serializedPayload, ...), and OffloadedPayload.inline(null) also rejects null — so the offload task fails and gets wrapped in a PayloadOffloadException.
Impact: any operation whose result serializes to null (a step/child/parallel/map branch returning null, waitForCondition state, or the handler's root output) previously checkpointed a null payload and replayed as null. With a global (or per-operation) offloader configured, these now throw at checkpoint time — a regression for a common case, and it is untested. resolve(null, ...)/deserialize already handle a null checkpoint, so the pipeline is only broken on the write side.
Fix: skip offloading when the serialized value is null so the legacy null behavior is preserved.
| var serialized = serDes.serialize(value); | |
| var effectiveOffloader = effectiveOffloader(offloader); | |
| if (effectiveOffloader == null) { | |
| return serialized; | |
| } | |
| var serialized = serDes.serialize(value); | |
| var effectiveOffloader = effectiveOffloader(offloader); | |
| if (serialized == null || effectiveOffloader == null) { | |
| return serialized; | |
| } |
| */ | ||
| public final class FileSystemPayloadOffloader implements PayloadOffloader { | ||
| private static final int CHECKPOINT_SIZE_LIMIT_BYTES = 256 * 1024; | ||
| private static final int OVERFLOW_THRESHOLD_BYTES = CHECKPOINT_SIZE_LIMIT_BYTES - 1024; |
There was a problem hiding this comment.
Claude AI review
OVERFLOW headroom is too small once enveloping/escaping is accounted for. offload decides inline-vs-reference from the raw serialized byte length against OVERFLOW_THRESHOLD_BYTES (256 KB − 1 KB). But when it returns OffloadedPayload.inline(...), PayloadCodec.serialize still wraps it as "@aws-durable-payload:v1:" + JSON({"mode":"INLINE","data":<serialized>, ...}). The serialized text is embedded as a JSON string, so every "/\ is escaped — for JSON payloads this can add well over 1 KB (worst case ~2x). A payload just under the 255 KB threshold can therefore produce a checkpoint value exceeding the 256 KB limit that OVERFLOW exists to stay under, causing the checkpoint to be rejected — the exact failure the mode is meant to prevent.
Fix: size the threshold against the enveloped form (e.g. offload if the enveloped/escaped size, or a conservative estimate such as raw*2 + fixed wrapper overhead, exceeds the limit) rather than subtracting a flat 1 KB from the raw length. Add a boundary test around the threshold to lock the behavior in.
Claude AI reviewPayload Offloader (Approach B) — reviewThe change is well-structured: a versioned Two confirmed defects, both surfacing only when an offloader is configured (the intended production setup):
Residual test risk: No test covers a Reviewed commit |
1 participant
Summary
This is the Approach B alternative to #648, which implements Approach A.
Testing
mvn spotless:applymvn testwith the Mockito Java agent required by this JDK environmentmvn clean installwith the same Mockito Java agentCloses #463