Skip to content
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

Revise queries #204

Merged

Conversation

sebastianburckhardt
Copy link
Member

Several changes (they are separated by commits):

  • rename StartOrchestration to ScheduleNewOrchestration
  • specify different defaults for clean entity storage (should clean everything by default)
  • support queries to (optionally) return stateless entities, and add more metadata to the returned entity metadata
  • make the page size a nullable int and keep it null if not specified (allows backend to choose an appropriate page size instead of using an arbitrary default)

@sebastianburckhardt sebastianburckhardt added P1 durable-entities Related to the Durable Entities support milestone labels Oct 5, 2023
Copy link
Member

@jviau jviau left a comment

Choose a reason for hiding this comment

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

Have some warnings to address due to the struct to class change.

/// even though the entity does not "logically" exist, in the sense that it has no application-defined state.
/// Stateless entities are usually transient. For example, they may be in the process of being created or deleted, or they may have been locked by a critical section.
/// </remarks>
public bool IncludeStateless { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

Having this in addition to IncludeState does not look the most user friendly. Maybe an enum to combine the two bools would work better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure that I prefer an enum since the two are logically independent (all four combinations are possible), and one of them is going to be relevant for most queries (IncludeState) while the other is an obscure feature likely to never be touched by most users (IncludeStateless).

Copy link
Member Author

Choose a reason for hiding this comment

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

I have renamed IncludeStateless to IncludeTransient as discussed.


/// <summary>
/// Gets a value indicating whether to release orphaned locks or not.
/// Gets a value indicating whether to release orphaned locks or not. Defaults to true.
/// </summary>
/// <remarks>
/// Locks are considered orphaned, and are released, and if the orchestration that holds them is not in state
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Locks are considered orphaned, and are released, and if the orchestration that holds them is not in state
/// Locks are considered orphaned, and are released, if the orchestration that holds them is not in state

@@ -6,29 +6,29 @@ namespace Microsoft.DurableTask.Client.Entities;
/// <summary>
/// Request struct for <see cref="DurableEntityClient.CleanEntityStorageAsync"/>.
/// </summary>
public readonly record struct CleanEntityStorageRequest
public record CleanEntityStorageRequest
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of changing to a class here, but not sure what a good alternative would be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I agree. I couldn't figure out how to get the defaults specified without turning it into a class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose I could define the struct without defaults and then define a static member Default which gets used as the default for the call. Let me try that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a static Default factory method as a way to specify the defaults and still keep this a struct.

/// Gets the default request parameters. The default is meant to represent
/// "maximal" cleaning that is safe to call at all times.
/// </summary>
public static CleanEntityStorageRequest Default => new()
Copy link
Member

Choose a reason for hiding this comment

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

You could also add a parameterless constructor and set all the bool properties to true there. This doesn't change default - but just an idea for you to consider.

@sebastianburckhardt sebastianburckhardt merged commit 895424d into feature/entities Oct 9, 2023
1 of 2 checks passed
@sebastianburckhardt sebastianburckhardt deleted the core-entities/revise-queries-and-defaults branch October 9, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
durable-entities Related to the Durable Entities support milestone P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants