Skip to content

Connect client entity SDK to gRPC service.#185

Merged
sebastianburckhardt merged 12 commits intofeature/entitiesfrom
entities-client
Sep 21, 2023
Merged

Connect client entity SDK to gRPC service.#185
sebastianburckhardt merged 12 commits intofeature/entitiesfrom
entities-client

Conversation

@sebastianburckhardt
Copy link
Member

Provides the glue to connect the entity client features to the gRPC service TaskHubSidecarService.

/// Gets the continuation token to continue the <see cref="CleanEntityStorageRequest"/>, if not null.
/// </summary>
public int OrphanedLocksRemoved { get; init; }
public string? ContinuationToken { 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.

Do we want to expose this to customers? Or do we want to abstract away the continuation behavior? As in, the gRPC protocol and implementation will handle continuation under the hood, only returning when it has exhausted continuations.

If you feel continuation must be exposed, we should at least offer an extension method or something that will loop through continuation. I wouldn't want every time a customer uses this API for them to have to include a while loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are some situations where exposing the continuation is desirable (e.g. user is running a durable orchestration to perform an extremely-long-running operation, so it can resume gracefully if interrupted).

But yeah, for the common case we do want to have the automatically-continuing one.

Copy link
Member Author

@sebastianburckhardt sebastianburckhardt Sep 14, 2023

Choose a reason for hiding this comment

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

I have added an optional argument to control whether the query should always continue until complete, or return a partial result with a continuation token.

Copy link
Member

Choose a reason for hiding this comment

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

I'll approve for now, but still not super happy with this.

I wonder if a custom IAsyncEnumerable<CleanEntityStorageResult>-esque type is the appropriate way? Then going one by one is as simple as iteration over the enumerable. Or going over all of them is an AggregateAsync()

So it would be a type like:

public abstract class AggregateRequest<TResult>
{
    public abstract Task<TResult?> NextAsync(); // make a single call

    public abstract Task<TResult?> AllAsync(); // invoke all of them
}

Just a thought, can be done at a later time (but before GA at least)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was also first considering something like AsyncPageable or AsyncEnumerable. However, I decided against it because I can really only see two common ways in which I expect this would be used (1) do all the work at once and wait for the final result (I expect this to be the most common so it should be the easiest to use) (2) do only one page (if users want to persist intermediate progress). For neither of those it provides significant value to return a pageable or enumerable result.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't blocking for the PR. We can think about it more later.

/// Gets the continuation token to continue the <see cref="CleanEntityStorageRequest"/>, if not null.
/// </summary>
public int OrphanedLocksRemoved { get; init; }
public string? ContinuationToken { 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.

I'll approve for now, but still not super happy with this.

I wonder if a custom IAsyncEnumerable<CleanEntityStorageResult>-esque type is the appropriate way? Then going one by one is as simple as iteration over the enumerable. Or going over all of them is an AggregateAsync()

So it would be a type like:

public abstract class AggregateRequest<TResult>
{
    public abstract Task<TResult?> NextAsync(); // make a single call

    public abstract Task<TResult?> AllAsync(); // invoke all of them
}

Just a thought, can be done at a later time (but before GA at least)

@sebastianburckhardt sebastianburckhardt merged commit cca8471 into feature/entities Sep 21, 2023
@sebastianburckhardt sebastianburckhardt deleted the entities-client branch September 21, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants