Revise how entity batches are executed and handle failures#954
Merged
sebastianburckhardt merged 5 commits intofeature/core-entitiesfrom Sep 8, 2023
Merged
Revise how entity batches are executed and handle failures#954sebastianburckhardt merged 5 commits intofeature/core-entitiesfrom
sebastianburckhardt merged 5 commits intofeature/core-entitiesfrom
Conversation
sebastianburckhardt
added
dt.core
davidmrdavid
reviewed
Sep 7, 2023
Comment on lines
27
to
31
| /// <summary> | ||
| /// The results of executing the operations in the batch. The length of this list must match | ||
| /// the size of the batch if all messages were processed; In particular, all execution errors must be reported as a result. | ||
| /// However, this list of results can be shorter than the list of operations if | ||
| /// some suffix of the operation list was skipped, e.g. due to shutdown, send throttling, or timeouts. | ||
| /// The results of executing the operations in the batch. If there were (non-application-level) errors, the length of this list may | ||
| /// be shorter than the number of requests. In that case, <see cref="FailureDetails"/> contains the reason why not all requests | ||
| /// were processed. | ||
| /// </summary> | ||
| public List<OperationResult>? Results { get; set; } |
Collaborator
There was a problem hiding this comment.
if there's less results and input operations, how do we know what results came from which operations? Or do we not need to associate them?
Collaborator
Author
There was a problem hiding this comment.
We always execute operations in order, so the returned results correspond to a prefix of the operations.
Co-authored-by: David Justo <david.justo.1996@gmail.com>
jviau
approved these changes
Sep 8, 2023
| OperationResult errorResult = new OperationResult() | ||
| { | ||
| Result = null, | ||
| ErrorMessage = "entity dispatch failed", |
Collaborator
There was a problem hiding this comment.
I am still unsure how this is supposed to be used. Does the presence of FailureDetails not already indicate an error occurred? Is ErrorMessage offering any new information?
Collaborator
Author
There was a problem hiding this comment.
I got the impression that FailureMessage is not sufficient as an indication because it is not used when using ErrorPropagationMode.SerializeExceptions. Also, this was designed to allow us to switch seamlessly from legacy DF (i.e. we an deserialize existing OperationResults that were created with the old version) in which case there are no FailureDetails present.
If FailureDetails were guaranteed to present it does not add significant value (not enough to justify its presence). Still, it can add just a little bit of value since there is often more information we want to include, and I don't think creating nested FailureDetails is always ideal since it can make it harder to find the original cause..
Co-authored-by: Jacob Viau <javia@microsoft.com>
…tity # Conflicts: # src/DurableTask.Core/Entities/TaskEntity.cs
sebastianburckhardt
added a commit
that referenced
this pull request
Oct 17, 2023
* Bring entity logic into DurableTask.Core (first milestone) (#887) * implementaton of entity mechanics, compatible with existing DF SDK, but without a user-facing entity SDK for DTFx * address PR feedback. * fix usings and namespaces * address PR feedback * address PR feedback (remove NameObjectManager), fix breaking change in TaskHubWorker, fix some comments * address PR feedback (fix CustomExceptionsTest, remove public property) * add #nullable enable to most new classes * address PR feedback * try to fix compiler errors * add a configuration setting that disables separate dispatch by default * address PR feedback * address PR feedback * fix semantic merge conflict. * Revise entity state and status format and access (#955) * update scheduler state and entity status format and helpers * fix mess-up caused by merge conflict * Revise entity message format and external access (#956) * revise how event messages are represented and used * fix merge anomaly. * make current critical section id publicly visible (#958) * remove orchestration tags from entity action (#952) * Rename OperationBatchRequest and OperationBatchResponse (#953) * rename OperationBatch to EntityBatch * fix accidentally commited change from another PR * Revise how entity batches are executed and handle failures (#954) * revise task entity definition * commit change that had been accidentally committed to a different PR. * Apply suggestions from code review Co-authored-by: David Justo <david.justo.1996@gmail.com> * Apply suggestions from code review Co-authored-by: Jacob Viau <javia@microsoft.com> --------- Co-authored-by: David Justo <david.justo.1996@gmail.com> Co-authored-by: Jacob Viau <javia@microsoft.com> * revise operation result encoding and add more comments. (#965) * revise entity backend properties and implement entity backend queries (#957) * revise entity backend properties and implement entity backend queries. * Minor revisions to querries and properties, and improved comments. * fix validation of which LockNext methods are being called. * improve comments * fix useage of IEntityOrchestrationService. * revise how to exclude entity results from queries. * address PR feedback * Update versions for ADO feed (#973) * Add no-warn for NU5104 (#974) * revise propagation path for entity parameters (#971) * fix propagation path for entity parameters that need to reach the orchestration executor. * address PR feedback. * Revise entity queries (#981) * rename includeDeletedEntities to includeStatelessEntities and add comment explaining the meaning * add backlogQueueSize and lockedBy to entity metadata * fix bugs in tracking store implementation (#979) * add scheduled start time parameter to the start-new-orchestration operation action. (#980) * Revise serialization of entitymessages (#972) * revise how entity messages are serialized when sent by orchestrators. * address PR feedback (use RawInput) * Rename includeStateless to includeTransient in entity queries (#985) * rename includeStateless to includeTransient * rename variable also * Rev to entities-preview.2 (#986) * fix null reference exception when running on older backends (#989) * Prepare for public preview (#994) --------- Co-authored-by: David Justo <david.justo.1996@gmail.com> Co-authored-by: Jacob Viau <javia@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are two changes here:
EntityExecutionOptionsfromTaskEntity