-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Base implementation for Cache Operations #2
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
Conversation
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, exc_traceback): |
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 also want to close the client too? Or does this take care of that
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 stub doesn't have a close. As per examples and documentation, the channel needs to be closed:
https://github.com/grpc/grpc/blob/v1.41.0/examples/python/helloworld/greeter_client.py#L29
| from . import _cache_service_errors_converter as error_converter | ||
|
|
||
|
|
||
| class CacheResult(Enum): |
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 like this, only Get needs a result
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.
Once we are over the SDK spikes, I would like to go cleanup other SDKs as well and potentially remove these results from MR2 responses as well.
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_value, exc_traceback): | ||
| self._secure_channel.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.
same here do we also want to close the client?
bruuuuuuuce
left a comment
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.
a few minor comments, but otherwise lgtm as a first pass
Uh oh!
There was an error while loading. Please reload this page.