Adding Throughput Bucket Header #40340
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for a new throughput bucket header to the SDK at both the request and client levels.
- Introduces the new "x-ms-cosmos-throughput-bucket" header in http_constants.py.
- Adds a new parameter "throughput_bucket" to various container and database creation/update functions.
- Updates internal request-building logic in _base.py to include the throughput bucket header.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/cosmos/azure-cosmos/azure/cosmos/http_constants.py | Adds the throughput bucket header constant. |
| sdk/cosmos/azure-cosmos/azure/cosmos/database.py | Updates methods to accept and propagate a throughput_bucket parameter; includes an unexpected import. |
| sdk/cosmos/azure-cosmos/azure/cosmos/cosmos_client.py | Adds throughput_bucket parameter support for client-level database creation. |
| sdk/cosmos/azure-cosmos/azure/cosmos/container.py | Adds throughput_bucket support in item creation methods. |
| sdk/cosmos/azure-cosmos/azure/cosmos/_base.py | Modifies header generation to include the throughput bucket if provided. |
Comments suppressed due to low confidence (1)
sdk/cosmos/azure-cosmos/azure/cosmos/http_constants.py:192
- [nitpick] Consider removing or clarifying the '#CHECK' comment to avoid leaving debugging markers in production code.
ThroughputBucket = "x-ms-cosmos-throughput-bucket" #CHECK
sdk/cosmos/azure-cosmos/azure/cosmos/_cosmos_client_connection.py
Outdated
Show resolved
Hide resolved
andrewmathew1
force-pushed
the
users/andrewmathew1/throughputbucket
branch
from
b424ad1 to
419e374
Compare
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
/azp run python - cosmos - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Great work @andrewmathew1
Everything looks good to me. Requesting some changes. Please do add samples as well and add a section in the readme.md that our sdk supports this feature and link to the sample on how to use it.
| @@ -171,6 +171,7 @@ class CosmosClient: # pylint: disable=client-accepts-api-version-keyword | |||
| level (to log all requests) or at a single request level. Requests will be logged at INFO level. | |||
| :keyword bool no_response_on_write: Indicates whether service should be instructed to skip sending | |||
| response payloads for write operations on items by default unless specified differently per operation. | |||
| :keyword int throughput_bucket: The desired throughput bucket for the client | |||
There was a problem hiding this comment.
The readme mentioned that the feature is in public preview? If that's the case, then everywhere this parameter is documented, it needs to be marked as provisional.
| @@ -265,6 +267,7 @@ async def create_database( | |||
| :paramtype offer_throughput: Union[int, ~azure.cosmos.ThroughputProperties] | |||
| :keyword dict[str, str] initial_headers: Initial headers to be sent as part of the request. | |||
| :keyword response_hook: A callable invoked with the response metadata. | |||
| :keyword int throughput_bucket: The desired throughput bucket for the client | |||
There was a problem hiding this comment.
It should be added to the per-method documentation that a value specified here will override any value already specified at the client-level.
There was a problem hiding this comment.
Also - it looks like this was maybe copy+pasted from the client-level documentation? This should be rephrased as it's not the bucket "for the client", rather it's the bucket "for the request" (or maybe "operation", as some of these, like query, are multiple requests).
* inital commit for throughput bucket * added test for throughput bucket * got rid of extra comment * sync part of throughput headers * added async part with new test file * added async part with tests * got rid of extra space * added _async label to test_headers_async * removed extra line * removed unnecessary syntax * made requested changes on pr, mostly for async tests * added another finally block to test_headers * edited replace container test * added sample files and section to the readme * got rid of trailing whitespace * added negative test, added to changelog * added header to changelog * got rid of extra , * edited readme for new sample file names * marked negative test as TODO --------- Co-authored-by: Andrew Mathew <[email protected]>
Description
Adding the throughput bucket header, available at the request and client level for both async and sync operations.
The throughput bucket header is able to be passed for every request on container and database operations. It's also able to be set at the overall client level. However, the header passed into the request will take precedence over the client level header if both are passed in.