fix: operations always return deserialized result#480
Conversation
zhongkechen
force-pushed
the
serdes
branch
4 times, most recently
from
313ec7c to
4d9d3c7
Compare
zhongkechen
changed the title
# Conflicts: # sdk/src/main/java/software/amazon/lambda/durable/operation/ChildContextOperation.java
| } | ||
|
|
||
| private void handleStepSucceeded(T result) { | ||
| var serializedResult = serializeAndDeserializeResult(result); |
There was a problem hiding this comment.
nit: serializedResult is a bit confusing, maybe parsedResult is better?
There was a problem hiding this comment.
any suggestions?
| // Send SUCCEED | ||
| var successUpdate = | ||
| OperationUpdate.builder().action(OperationAction.SUCCEED).payload(serializeResult(result)); | ||
| OperationUpdate.builder().action(OperationAction.SUCCEED).payload(serializedResult.serialized()); |
There was a problem hiding this comment.
Are we using serialized here because we re-execute step during replay? why can't we use raw then?
There was a problem hiding this comment.
payload is a string field
| return resultSerDes.serialize(result); | ||
| var serialized = resultSerDes.serialize(result); | ||
| if (shouldValidateSerializationRoundTrip()) { | ||
| deserializeResult(serialized); |
There was a problem hiding this comment.
will it call the deserialize twice? i.e. if they write to s3 during deserialization, the context will be write twice?
There was a problem hiding this comment.
Yes, deserialize will be called again each time users call get(). We can improve this by having a cached value for get()
| private void checkpointSuccess(T result) { | ||
| var serialized = serializeResult(result); | ||
|
|
||
| private void checkpointSuccess(T result, String serialized) { |
There was a problem hiding this comment.
T result is this required?
There was a problem hiding this comment.
yes, when caching large payload
3 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Fix #456
Description
Call deserialize immediately after serialize to make sure the serialized result is always deserialize-able
Demo/Screenshots
Checklist
Testing
Unit Tests
Have unit tests been written for these changes?
Integration Tests
Have integration tests been written for these changes?
Examples
Has a new example been added for the change? (if applicable)