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
feat: add pooled grpc transport #748
feat: add pooled grpc transport #748
Changes from 92 commits
8c5fd7d
393860c
11b3493
1ac0b7a
cf981d8
5ea2bc3
84fd9c3
43b17dd
32e8e45
65cb219
c8b8a5a
ff835ec
3207ef5
3b3d720
fa29ba1
4e792a5
35e8a58
dfab801
72f7d0e
be3de7a
f6c7f36
44e76c1
c4d537e
99a49a4
bd4fb5e
6b13c29
60841c5
0a3086c
08c3c42
05e10cd
895093f
1684274
75d276a
92752c0
79c82c3
f3b7fbd
aa37a31
17d731f
a0a5c57
64a05d8
741147d
005900c
d58fc74
9983e18
bfeb546
dba7a3c
3ae6722
b9f2b0d
e483370
136a8fe
af86f6b
cbe7062
909f889
5efa0ac
2b322e2
d4904d7
6917593
7b5ecbb
c0616dd
983d4c7
c19658a
c2d0da0
8b54a30
0dd981b
b0ecd3c
e47551f
96d526b
e997892
5c86f57
3bc4131
3c4e0b6
197bf95
9d8122b
2632b70
d4e052b
1aa694b
e2d4bd5
a91362f
d80a8c0
b888ee8
94c1187
4c02e6c
d65b432
4b63d87
4ccc421
8001240
7c9cea7
3bbebea
8bff9d0
4ae2146
b9dc2f7
19036d8
8a22d15
38e5662
5155800
522f7fa
74029c9
7f2be30
2b044ce
1743098
e5fa4b6
8955ec5
002bc5f
65f0d2f
dbf19c9
9f3e0c5
a0620ea
1486d5a
28d5a7a
70fbff9
383d8eb
018fe03
f0403e7
cb23d32
bc31ab8
f54dfde
46cfc49
573bbd1
4f2657d
59955be
377a8c9
8a29898
50aa5ba
42a52a3
50dc608
b116755
ec5eb07
55cdcc2
9e3b411
ab43138
cb1884d
9a89d74
1e62c71
d70c685
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.
Please avoid making the pool fixed size. It can have negative effects on customers with fluctuating traffic:
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 have any examples of libraries that implement dynamic pooling like you're thinking? What sort of algorithm do you have in mind?
From what I saw, it seems the other libraries generally use fixed pool sizes, so I was hoping to keep it simple for now, and then revisit dynamic sizing after the core client is complete
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.
You can take a look at the java implementation: https://github.com/googleapis/gapic-generator-java/blob/main/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java#L248
However I think it's ok to have a fix sized pool to start with, just make it extensible so that in the future we can make it dynamic?
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 would be nice to await all of the channels to be warm before letting endusers use the client
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.
The problem is we don't have any instances to warm when the client is created, only when we start creating tables
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.
should this be exposed to end users?
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 part of the Client spec used by all GCP libraries. The reason I override it here was to change the type annotation
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.
should there a deregister call as well?
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'm not sure I understand. This task is needed because we can't call await from
__init__
, so we schedule it as a background task insteadThere 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.
why not bubble up this error?
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've gone back and forth on it
The question comes down to whether we should support creating these classes outside async context:
Let me know if you have thoughts
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.
Is there any way to opt out of rest for the data client?
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.
Yes, I believe it's configured in the yaml somewhere. We can address at that in a future PR
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.
@igorbernstein2 Can you say more about that? We have gotten some customer feedback in other product areas that REST transport is important to them in some use cases, so we've typically preferred to include the option when we can and let users choose which they require with sensible defaults. Are you suggesting we disallow it entirely? (I see the previous cl/458022604 where this was disallowed, but I'm wondering if those are still relevant.)
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.
@meredithslota the rest transport doesn't support asyncio, so I think it makes sense to hard-code the transport to use our pooled transport for at least the async layer.
When it comes time to add the synchronous layer, we could support rest if customers find it useful, but it just wouldn't be able to make use of the same channel management optimizations that we're adding to grpc side. Maybe @igorbernstein2 has some more context though