ArangoDB-Community / arangodb-net-standard

A consistent, comprehensive, minimal interface to enable .NET applications to use the complete range of features exposed by the ArangoDB REST API.
Apache License 2.0
76 stars 30 forks source link

Support for Overload Control #431

Closed tjoubert closed 1 year ago

tjoubert commented 2 years ago

https://www.arangodb.com/docs/stable/http/general.html#overload-control

@DiscoPYF , there is a requirement to support the Overload Control feature in ArangoDB. To specify a time limit (in seconds) for a request to remain in the queue, the client can pass the x-arango-max-queue-time-seconds header. The server also returns a header (all the time) called x-arango-queue-time-seconds, indicating the number of seconds a request was queued before execution.

To support the request header, I've added it as one of the properties, QueueTimeLimit, in ApiHeaderProperties. I will then add an optional parameter, ApiHeaderProperties headerProperties = null, to every method that makes an API call (except overloaded convenience methods). Let me know your thoughts about this approach please.

To support the response header, I've added a new class ApiResponseHeaders which extracts header values from the HttpResponseHeaders returned by the API. I've also added one generic ApiResponseBase class (along with its interface IApiResponseBase) which has a property public ApiResponseHeaders ResponseHeaders { get; set; }. Every response class in the library will inherit from ApiResponseBase or IApiResponseBase. Please check my examples in GetAllViewsResponse and ViewResponse. Also, take a look at how I'm setting ApiResponseBase.ResponseHeaders in ViewApiClient.GetAllViewsAsync() and ViewApiClient.PostCreateViewAsync(), after the response deserialization.

What do you think about my approach here? Feel free to suggest changes. I'll adapt and apply it to the whole library. I do realize that this may be a breaking change. Ideally, every method should return a response object, which means that we'll need to change signatures for some methods like IPregelApiClient.PostStartJobAsync() or maybe we'll overload them.

I'm looking forward to your feedback.

DiscoPYF commented 2 years ago

I'm starting to think that we should have added one more level of abstration with all responses based on a generic response object.

That's not technically "adding" a level of abstration, but you get the idea. All responses are standardized but the downside is that callers would have to access one more property from the reponse object (.Content). (And being a breaking change).

Some code generators provide several method signatures for various level of information. For example with OpenAPI Generator, you get generated code like:

public interface MyClient
{
    Task DoSomethingAsync();
    Task DoSomethingWithHttpInfoAsync();
}

But doing this manually would be difficult.

rossmills99 commented 2 years ago

Another possibile alternative (not necessarily better) is to provide additional method overloads that provide the additional response information via an out param. One example might be:

public virtual async Task<GetAllViewsResponse> GetAllViewsAsync(
    out ApiResponseHeaders ResponseHeaders,
    CancellationToken token = default) 
{
... etc... 
}

Just offering as an alternative for consideration, it could help as a way to expose these properties in cases that don't suit the inherited base class, like @DiscoPYF mentioned. I'm also tired from jetlag though, so take this idea with that in mind!

DiscoPYF commented 2 years ago

provide additional method overloads that provide the additional response information via an out param.

Unfortunately, async methods can't have out parameters. https://stackoverflow.com/questions/18716928/how-to-write-an-async-method-with-out-parameter

tjoubert commented 1 year ago

Closing this PR and will create new ones for request and response