-
Notifications
You must be signed in to change notification settings - Fork 44
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
Sushisource
left a comment
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
Sushisource
left a comment
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 themselvesISerializationContextas a base/marker interface for the contexts and nestedActivityandWorkflowclass implementations of itChecklist