Skip to content

Serialization context for converters and codecs #446

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

Merged
merged 3 commits into from
Apr 15, 2025

Conversation

cretz
Copy link
Member

@cretz cretz commented Apr 9, 2025

What was changed

  • Added IWithSerializationContext<T> that can be implemented by data converters (and is), payload converters, encoding-specific payload converters, failure converters, and payload codecs to provide context-specific versions of themselves
  • Added ISerializationContext as a base/marker interface for the contexts and nested Activity and Workflow class implementations of it
  • Updated workflow client, schedule client, async activity client, workflow worker, activity worker, workflow outbound child, and workflow outbound external code to use context-specific converters/codecs.
  • Added test to test most of those paths

Checklist

  1. Closes [Feature Request] Serialization context for codecs and converters #438

@cretz cretz requested a review from a team April 9, 2025 13:22
}
}

private async Task HandleCacheEvictionAsync(WorkflowActivation act, RemoveFromCache job)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we added data conversion details to the existing HandleActivationAsync it complicated the code a lot because eviction was inside it too. So we took this opportunity to refactor and move eviction here to a separate method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The royal "we" here is a bit hilarious lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I have multiple personality disorder

@cretz cretz force-pushed the serialization-context branch from ba9b716 to 5608843 Compare April 9, 2025 13:28
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, definitely a bit of a laborious change.

I'm tempted to say it could maybe be a bit cleaner by creating some extension methods like asSerializationContext on things like some of the activation jobs/inputs/etc so that the places you're extracting info from them and turning them into a serialization context can be somewhat-deduped, and, maybe more importantly, just not create so much inline noise in the places where they're happening.

That's stylistic though and nonblocking

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was supposed to be approve

@cretz
Copy link
Member Author

cretz commented Apr 10, 2025

Yeah most of that inline noise is just .NET taking so many characters to type, not sure having single-use helpers will help too much. Yes this is a somewhat large effort, as it was in Java a couple years ago, and it's going to be really rough for whoever has to do this work in Python.

…-context

# Conflicts:
#	src/Temporalio/Client/WorkflowExecution.cs
#	src/Temporalio/Client/WorkflowHandle.cs
#	tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs
@cretz cretz merged commit 6dbf752 into temporalio:main Apr 15, 2025
9 checks passed
@cretz cretz deleted the serialization-context branch April 15, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Serialization context for codecs and converters
2 participants