cluster: add application_info #486
Conversation
cassandra/cluster.py
Outdated
| application_info = None | ||
| """ | ||
| An instance of :class:`.cluster.ApplicationInfo` or one of its subclasses. | ||
|
|
||
| Defaults to None | ||
|
|
||
| When not None makes driver sends information about application that uses driver in startup frame | ||
| """ |
There was a problem hiding this comment.
Can we make the field private (I mean name starting with underscore)? It will be clearer that it is not meant to be edited by user, but only set using the constructor.
There was a problem hiding this comment.
Also please add a type hint for the field.
cassandra/cluster.py
Outdated
| class ApplicationInfo(object): | ||
| def __init__(self, application_name=None, application_version=None, client_id=None): | ||
| if not isinstance(application_name, str): | ||
| raise TypeError('application_name must be a string') | ||
| if not isinstance(application_version, str): | ||
| raise TypeError('application_version must be a string') | ||
| if not isinstance(client_id, str): | ||
| raise TypeError('client_id must be a string') | ||
| self.application_name = application_name | ||
| self.application_version = application_version | ||
| self.client_id = client_id |
There was a problem hiding this comment.
I don't understand the purpose of this logic. You are defaulting all the args to None, but in the body you require them to be str, so the constructor will fail unless all the args are provided. Why not make them mandatory in this case? Or change the body to allow None - I don't know which semantics you intended here.
Also, could you add type hints? I know rest of the driver doesn't have them, but we can use them in new code to aid users.
There was a problem hiding this comment.
fixed, added type hints.
cassandra/cluster.py
Outdated
| if application_info is not None: | ||
| if isinstance(application_info, type): | ||
| raise TypeError("application_info should not be a class, it should be an instance of ApplicationInfo class") | ||
| if not isinstance(application_info, ApplicationInfo): | ||
| raise TypeError( | ||
| "application_info should be an instance of ApplicationInfo class") | ||
| self.application_info = application_info | ||
|
|
There was a problem hiding this comment.
Why do you have two checks? What is the situation where the first check (isinstance(application_info, type)) will be triggered, but second one won't?
There was a problem hiding this comment.
What was the purpose of having two checks? You must have had some reason to do this, and I'd like to understand it.
cassandra/connection.py
Outdated
| self.orphaned_request_ids = set() | ||
| self._on_orphaned_stream_released = on_orphaned_stream_released | ||
| self.application_info = application_info | ||
|
|
||
| if ssl_options: |
There was a problem hiding this comment.
Could you make the field private (underscore), and add it to the class (with type hint)?
cassandra/connection.py
Outdated
| if self.application_info: | ||
| if self.application_info.application_name: | ||
| opts['APPLICATION_NAME'] = self.application_info.application_name | ||
| if self.application_info.application_version: | ||
| opts['APPLICATION_VERSION'] = self.application_info.application_version | ||
| if self.application_info.client_id: | ||
| opts['CLIENT_ID'] = self.application_info.client_id |
There was a problem hiding this comment.
Maybe a bit cleaner way would be to have a (private) method on ApplicationInfo that takes a dict and sets those values? That way Connection doesn't need to know ApplicationInfo internals.
There was a problem hiding this comment.
good idea, done.
8c02393 to
d7d7385
Compare
Implement clustr.application_info to make driver send following startup options to server: 1. `APPLICATION_NAME` - ID what application is using driver, example: repo of the application 2. `APPLICATION_VERSION` - Version of the application, example: release version or commit id of the application 3. `CLIENT_ID` - unique id of the client instance, example: pod name All strings.
d7d7385 to
7e0a9d2
Compare
Implement clustr.application_info to make driver send following startup options to server:
APPLICATION_NAME- ID what application is using driver, example: repo of the applicationAPPLICATION_VERSION- Version of the application, example: release version or commit id of the applicationCLIENT_ID- unique id of the client instance, example: pod nameAll strings.
Fix: #485
Results:
Pre-review checklist
I added relevant tests for new features and bug fixes.I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.