temporalio / sdk-java

Temporal Java SDK
https://temporal.io
Apache License 2.0
200 stars 134 forks source link

RunID is needed oftimes when relying on HasWorkflowSerializationContext in a PayloadCodec or Converter #1994

Closed mnichols closed 3 months ago

mnichols commented 4 months ago

Is your feature request related to a problem? Please describe. RunId is not included in HasWorkflowSerializationContext. This makes it hard for PayloadCodec implementations that are caching payloads to disambiguate execution runs that have been ContinuedAsNew, Reset, etc.

Describe the solution you'd like Extend the implementations of HasWorkflowSerializationContext to accept RunId where possible. This is possible to set in all cases except ClientInterceptor concerns, so can be null.

Describe alternatives you've considered The concrete case this solves is where a user must set the TTL to expire payloads based on retention policy, but if an execution has been ContinuedAsNew only the payloads which correspond to the Runs which have been Completed/Canceled should be expired. Marking the entire payload history as expired by WorkflowID would be incorrect since the latest run might remain open for some time.

More broadly, having the RunID is so common across the execution info we expose elsewhere, it makes sense to pass this important identification of which run we are dealing with all the way down here as well.

Quinn-With-Two-Ns commented 4 months ago

Extend the implementations of HasWorkflowSerializationContext to accept RunId where possible. This is possible to set in all cases except ClientInterceptor concerns,

It is true that a workflow context has a runID, but it will often not be the correct runID. For example in the continue as new case the SDK does not know the runID of the new workflow execution.

The concrete case this solves is where a user must set the TTL to expire payloads based on retention policy

Based on this use case I am not sure this is an appropriate solution given the SDK does not always have visibility into a workflows lifecycle. This would require to have some server sent events to fully capture a workflow/activity lifecycle to make decisions on payloads

https://github.com/temporalio/temporal/issues/3709

SerializationContext is meant to provide information needed to serialize a payload, if that information cannot be consistently provided then it can't be used to serialize the payload and I don't think it belongs in SerializationContext

mnichols commented 4 months ago

I actually implemented the changes last night to see if RunId was available and apart from the Client interceptor found it was always there, but is this an example of where the runId being passed in might not be the correct runID??:

https://github.com/temporalio/sdk-java/blob/hasworkflowserialization-context-runid/temporal-sdk/src/main/java/io/temporal/internal/sync/SyncWorkflowContext.java#L140

Or is it in the POJO bit here: https://github.com/temporalio/sdk-java/blob/hasworkflowserialization-context-runid/temporal-sdk/src/main/java/io/temporal/internal/sync/POJOWorkflowImplementationFactory.java#L260 ?

I'm trying to learn where the wrong runID might be getting inserted into this instance.

Quinn-With-Two-Ns commented 4 months ago

I actually implemented the changes last night to see if RunId was available and apart from the Client interceptor found it was always there, but is this an example of where the runId being passed in might not be the correct runID??:

Yes, any case involving reset, continue as new, last workflow result or child workflows you will get the runID for the current execution not the new execution since the runID is generated on the server.

mnichols commented 3 months ago

Closing this out as it is not a tenable solution