Include both protocol method and grow-up method will break user

[pshao25]

Describe the issue or request
We are considering adding both protocol method and grow-up method into DPG client. Eg.,

// protocol method
public virtual async Task<Response> GetModelAsync(string mode, RequestContext context = null); 
// grow-up method.
public virtual async Task<Response<Model>> GetModelAsync(string mode, CancellationToken cancellationToken = default); 

That will break if user calls GetModelAsync("mode") without RequestContext and CancellationToken.

This issue does not exist in post. Eg.,

// protocol method
public virtual async Task<Response> PostModelAsync(RequestContent requestContent, RequestContext context = null); 
// grow-up method.
public virtual async Task<Response<Model>> PostModelAsync(Input mode, CancellationToken cancellationToken = default); 

[annelo-msft]

In this case, if we are growing up the client from a DPG client, we will change the name of the grow-up method. The recommended rename is GetModelValueAsync -- since there is a requirement that convenience methods will be reviewed by arch board instead of GA'ed, the arch board will review the new method name.

If we are adding protocol methods to an existing GA HLC, we wouldn't be able to change the name of the convenience method, we would need to change the name of the protocol method. We could do this via C# generator transforms.

@KrzysztofCwalina, @ellismg FYI

[KrzysztofCwalina]

I suggest that in cases where the only difference in overloads is RequestContext vs CancellationToken, we don't add the CancellationToken parameter and make the model castable from Response, i.e. I would do:

// protocol method
public virtual async Task<Response> GetModelAsync(string mode, RequestContext context = null); 
// grow-up method.
public virtual async Task<Response<Model>> GetModelAsync(string mode); 

Users who want to pass a CancellationToken would do:

Response response = clinet.GetModelAsync(mode, new RequestContxt(token));
var model = (Model)response;

In general, I think we are overdoing the CancellationToken parameters in the Azure SDK. In .NET, rarely used parameters are typically not added to helper overloads.

[annelo-msft]

@KrzysztofCwalina, isn't making the model castable from Response in the #3 bucket that we will address after we have completed #4, e.g. the full grow-up story?

I am fine not adding the CancellationToken in the overload, but I think in this case we would be saying that we're not allowing CancellationToken to be passed unless you use the protocol method, and the model cast functionality will come in a later release.

[annelo-msft]

@KrzysztofCwalina, is the implication of this that we want to avoid using CancellationToken in the method signatures of all grow-up methods? That would change what is documented here: https://github.com/ellismg/design-notes/blob/main/llc/llc-evolution.md#developer-driven-evolution

[KrzysztofCwalina]

isn't making the model castable from Response in the #3 bucket that we will address after we have completed #4, e.g. the full grow-up story?

The way I think about it is that we postponed the official support, exploring all corner cases (e.g. casing untyped LROs to typed LROs), etc. But I think it would be totally reasonable to provide informal guidance to teams who run into simple cases like this one, unless we have a concrete concern about the informal guidance.

[KrzysztofCwalina]

is the implication of this that we want to avoid using CancellationToken in the method signatures of all grow-up methods?

I would not go as far. The only thing I am saying is that not all helper overloads need to take cancellation tokens given the CTs are optional parameters anyway, and we know these parameters are not used in mainline scenarios. The intention of the guidelines that talk about adding cancellation tokens to all methods was that there should always be a way to cancel SDK service method calls by passing a CT, not that literally every overload takes a CTs. The latter is very not-.NET-like. In .NET rarely used optional parameters are not added to signatures of helper overloads.

[annelo-msft]

The only thing I am saying is that not all helper overloads need to take cancellation tokens

It feels a little arbitrary, though, that most grow-up methods would take an optional CancellationToken, but this specific case with no input model does not. Would it possible to be crisp on the thinking behind when we take an optional CT parameter and when we don't?

[annelo-msft]

Re-opening per lack of clear guidance.

[KrzysztofCwalina]

What about this guidance:

1. If we run into overload conflict like described in this thread, it's ok to skip the CT from the only grow-up method.
2. If there is no overload conflict, and only one grow up method, the grow up method should take [optional] CT.
3. If there are more than one grow up methods that differ by the number of optional parameters, only the longest overload should take CT.