-
Notifications
You must be signed in to change notification settings - Fork 284
Extended sessions for entities in .NET isolated #3256
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
base: dev
Are you sure you want to change the base?
Conversation
| /// Configuration settings for RemoteOrchestratorContext in out-of-process mode, transmitted via gRPC. | ||
| /// </summary> | ||
| public class RemoteOrchestratorConfiguration | ||
| public class RemoteInstanceConfiguration |
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.
This is another case of a breaking API change. Is it safe to make?
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.
Checked with @nytian, this class is actually not used outside of this repo so this should be a safe change. I'll actually go ahead and make it an internal class in this PR too.
| /// True by default. | ||
| /// </summary> | ||
| public bool IncludePastEvents { get; set; } = true; | ||
| public bool IncludeState { get; set; } = true; |
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.
I'm wondering whether this breaking change is safe. Does this property get used by anything, either directly or indirectly? An example of indirect usage would be something like it gets set by deserializing configuration in host.json (I'm not sure if it does, but it looks like something that potentially could be).
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.
This property was introduced by me in the PR for extended sessions for orchestrations, so it shouldn't be getting used by anything outside of this repo. I think it should be a safe change to make.
Which does beg the question why I made it a public property... probably something I need to change in this PR.
This PR introduces the extended sessions feature for entities in .NET isolated. It essentially copies the existing logic for extended sessions for orchestrations and makes certain properties/classes more generic for shared logic between the two.
EntityTriggerAttributeBindingProvider: Passes theRemoteEntityContextconfiguration to the proto converter so that the entity state is only attached to the proto request if necessary (extended sessions are not enabled, or this is the first execution within an extended session)OrchestrationTriggerAttributeBindingProvider: Changed to reference the more generic propertyIncludeState(previouslyIncludePastEvents)RemoteEntityContext: Introduced a newConfigurationsfield to pass information about extended sessions to the workerRemoteOrchestratorContext: Changed to reference the more genericRemoteInstanceConfigurationandIncludeStateobjects/propertiesOutOfProcMiddleware: Existing orchestration logic changed to reference the more genericWorkItemMetadata.IncludeStatefield, and logic introduces for entities to extract information from theWorkItemMetadataobject and attach it to theRemoteEntityContext, as well as to throw aSessionAbortedExceptionif the worker needed an entity state and none was attached to the requestProtobufUtils: Changed to only include entity state in the request if necessary, and to serialize a map of values containing information about extended sessions for the worker processRemoteInstanceConfiguration: Renamed version ofRemoteOrchestratorConfigurationto be used by both orchestrations and entitiesDurableFunctionExecutor.Entity: Changed to pass the extended sessions cache to theTaskEntityDispatcherTaskEntityDispatcher: Changed to pass the cache toGrpcEntityRunnerPull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.