-
Notifications
You must be signed in to change notification settings - Fork 38
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
Conversation
} | ||
} | ||
|
||
private async Task HandleCacheEvictionAsync(WorkflowActivation act, RemoveFromCache job) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
ba9b716
to
5608843
Compare
There was a problem hiding this 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
There was a problem hiding this 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
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
What was changed
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 themselvesISerializationContext
as a base/marker interface for the contexts and nestedActivity
andWorkflow
class implementations of itChecklist