-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update Protobuf and GCP dependencies in Beam Python SDK #24599
Changes from 22 commits
a3e72b7
673aaf3
ee1da36
6140ff7
8f24747
e6af718
9c44141
5860711
fd4176f
2cf5526
afbe42d
ab20544
990cf94
7eab925
073788d
15a4d3b
30b9b7f
882e5af
bf2846c
3a0c1b7
2614d0e
188a5d6
2dba13f
b5ac388
0045b89
9b2f0de
35631f5
1d93010
3801c16
8d25563
3e95526
de15ba2
eac41bb
853fc86
1ebc815
7bfece4
7552e3d
01d7898
15f47d8
cb23020
7b4d9f5
caa04aa
bf78ba6
b7a1807
cb8d016
650827f
f522b38
7c4d268
297b7f4
83c4477
e7844e6
7eee7b7
adf34fb
adb7d30
0070723
c542de5
8deb83c
345f780
2f94ddd
5bafc65
dd6dbec
bc58086
e35bdef
84aeff4
b617f60
2ce87e7
334846a
9456a33
15db416
2558b6b
454481e
af2de21
6893398
f72eaa3
a29505f
ba8b376
5534073
a678bc4
0f0c3ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,3 +137,22 @@ def report_latency(self, now, latency_ms, num_mutations): | |
num_mutations: int, number of mutations contained in the RPC. | ||
""" | ||
self._commit_time_per_entity_ms.add(now, latency_ms / num_mutations) | ||
|
||
|
||
def extract_byte_size(proto_message): | ||
""" | ||
Gets the byte size from a google.protobuf or proto-plus message | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's increase the lower bound on Datastore and assume proto-plus messages for simplicity. the v2 dependency has been available for more than 1 year now, I don't think we need to support v1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know of any reason to not stop supporting v1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some helper functions which converts key or entity to protobuf. There are certain piece of test code that replicates this behavior using To replace the FakeMutation class, we need to refactor some pieces of code. So for now, I am keeping this as it is now and I filed an issue #25625 to tackle this not only for this library but for other GCP dependencies as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
google-cloud-datastore moved from using protobuf to using | ||
proto-plus messages. | ||
protobuf object has attribute ByteSize() but proto.Message() objects | ||
don't. Workaround: | ||
https://github.com/googleapis/proto-plus-python/issues/163 | ||
""" | ||
if hasattr(proto_message, "ByteSize"): | ||
# google.protobuf message | ||
return proto_message.ByteSize() | ||
if hasattr(type(proto_message), "pb"): | ||
# proto-plus message | ||
return type(proto_message).pb(proto_message).ByteSize() | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throw an exception here? |
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 still need this helper function?
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.
No. Removed it