ThroughputBucketing: Adds Throughput Bucket to RequestOptions and ClientOptions #4940
Conversation
Achint-Agrawal
requested review from
khdang,
sboshra,
adityasa,
neildsh,
kirankumarkolli,
FabianMeiswinkel,
kirillg and
Pilchie
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Achint-Agrawal
changed the title
Achint-Agrawal
changed the title
Achint-Agrawal
changed the title
Achint-Agrawal
changed the title
| /// If <see cref="AllowBulkExecution"/> is set to true in CosmosClientOptions, throughput bucket set on the CosmosClient is used. | ||
| /// </remarks> | ||
| /// <seealso href="https://aka.ms/cosmsodb-bucketing"/> | ||
| public int? ThroughputBucket { get; set; } |
There was a problem hiding this comment.
Is an int the only possible way to identify a throughput bucket or can names/identifiers be also used? The int/number seems like it will cause confusion for customers easily.
There was a problem hiding this comment.
How is this feature rolled-out? If there is any opt-in necessary on the service side in .Net SDK this should only be released in the preview nuget package first - and at least the PR description should have clear instructions for onboarding. If the feature is just enabled automatically for all accounts we can add this as public API assuming we have confidence in the public API in the normal nuget packaage (assuming all deployements including non-public clouds have been finished to enable the backend feature)
| /// </summary> | ||
| /// <remarks> | ||
| /// If throughput bucket is also set at request level in <see cref="RequestOptions.ThroughputBucket"/>, that throughput bucket is used. | ||
| /// If <see cref="WithBulkExecution(bool)"/> is set to true, throughput bucket set on the CosmosClient is used. |
There was a problem hiding this comment.
Why this constraint? That seems super confusing. I get what you are saying - if bulk mode is enabled and request options have different values this is a challenge in .Net - but why not throw an ArgumentException if request options has throughput bucket defined and bulk mode is enabled - that way the contract is enforced cleanly
| @@ -940,6 +940,16 @@ public IFaultInjector FaultInjector | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
Please also add an aka.ms link to explain the concept and constraints of throughput buckets
|
|
||
| if (options.ThroughputBucket.HasValue) | ||
| { | ||
| headers.Set(HttpConstants.HttpHeaders.ThroughputBucket, options.ThroughputBucket.ToString()); |
There was a problem hiding this comment.
I know we only support a small number of buckets, but we should really specify a format here to not have something like thousands separators vary by client locale settings.
| /// Set the ThroughputBucket in the request headers | ||
| /// </summary> | ||
| /// <param name="requestMessage"></param> | ||
| private void SetThroughputBucket(RequestMessage requestMessage) |
There was a problem hiding this comment.
[Non blocker]
Our RequestOptions are stateless but pattern of overrides implementation in RequestInvokerHandler is odd.
One alternative way is to pass the CosmosClientOptions along with ReqeustMessge for RequestOptions.RequestOptions?
/cc: @FabianMeiswinkel thoughts?
Description
This PR add ThroughputBucket in RequestOptions and ClientOptions to support Throughput Bucketing
Type of change
Please delete options that are not relevant.