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 test proxy #836
feat: add test proxy #836
Changes from 1 commit
aa1e40e
b059139
3ed2168
5199839
4e14bf3
cbb95c9
f43aac1
91fc1e6
06e5276
62b8e48
237e051
868ff2e
21a5077
02f0c09
456caba
f3627c1
bcc02d7
5e7c156
604d3d8
14f359d
858c57d
36a3153
07b39b1
5d90478
b69da5a
df3ea47
8dcd444
94a8684
72b8d1b
8496211
71ba0ea
94e98db
320d157
5064870
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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 will record the last exception's code. Is it intended? If so, does it correspond to the first exception occurrence (i.e., the head/root of the exception chain)?
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, we make use of exception groups so that we can expose a set of retried errors together with the final error. The exceptions are wrapped in a
bigtable.RetryException
. The final exception in the group should be the most relevant oneMore details here: #825
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.
what's the expected unit/format? The implementation seems to imply that it's integer?
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.
for the timeouts? They are float or None, but python accepts ints as floats
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.
Does it mean seconds or milliseconds?
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's in seconds. It's better documented here:
python-bigtable/google/cloud/bigtable/data/_async/client.py
Line 574 in aa760b2
Does that match the data sent from the tests?
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.
Does it have real effect on the client? I wonder if no-op here can also work?
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's been a while since I looked at this, but I think this is the one spot where I couldn't get a test to pass without modifications to the test code
I think the library will raise an exception if I try to create an empty SetCell mutation, but I believe the tests expect it. I was going to suggest we change the example mutation in the conformance tests to one with valid data
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 see. You are right.. But somehow they other clients seems to be fine with the empty set. You can keep the line as is. I will make the test change later and let you know when it's done.