Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Cosmos] CosmosDB asynchronous client #21404
[Cosmos] CosmosDB asynchronous client #21404
Changes from 55 commits
61ba8d1
15dcceb
bda95c3
c9648ab
80540dc
1285438
992b0cd
47cb688
f3fa79f
0c49739
47f4af5
c97c946
fcd95db
36c5b90
44db2a2
ec5b6ed
d63d052
5d74c8f
89fc2f7
fdaa880
3f9baf2
d6650bc
043dfe0
befdb41
8cffbe2
72de7c8
5b805b8
8d8d0c4
b597ca8
18319df
162c44d
ebbac51
43f78e6
470aa5b
74da690
7104d63
20718c7
d825eaa
e3c27a5
3b778ad
c6e352e
8971a25
52736ac
ad98039
f76c595
3f02a65
e719869
d03ee05
02c52ee
2cb4551
cf20d35
f456817
ea9bd16
709d2eb
3277dd8
a57cb4d
014578b
0d79695
fdabea1
016d0dd
8228aa9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants such as these should be in all caps: https://www.python.org/dev/peps/pep-0008/#constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simorenoh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be passed around safely, but needs to be manually disposed of once you're done using it - the alternative to this being to use the client using a
with
statement:async with CosmosClient(url, key) as client:
and then starting your cosmos db logic within that contextThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simorenoh What happens if these async clients are not closed properly? Memory leak or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have to test, but I assume that is the case - you also get an error in your code stating the client was not properly disposed of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto. I was a bit confused when I saw these being referred in the function below without realizing they are constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to close the client here too?
await client.close()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is that second way to initialize I had mentioned above - the way it is done here, it gets disposed once the
with
statement gets exitedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async for
loop right after - any function that has eitherawait
orasync
within has to be defined as async by pythonThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto on the constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to close the client in this example too? I'm not seeing "with" being used either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we are forking the implementation code and copying from the non async io code path, we need to have a plan on maintenance.
now if there is any bug let's say in the query stack we need to fix this into different codebase.
multiple options to consider for future:
@kushagraThapar