refactor: rest client#217
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the write path to use a new synchronous RestClient (urllib3-based) instead of the previous generated client/write service wiring, and updates InfluxDBClient3 construction accordingly.
Changes:
- Refactors
WriteApito issue HTTP requests via a newRestClient, adds gzip decision/compression utilities, and adds endpoint/exception translation logic. - Introduces
influxdb_client_3/write_client/_sync/rest_client.pyto encapsulate urllib3 request/response handling. - Updates
InfluxDBClient3initialization to pass write connection details (base_url/auth/gzip) directly intoWriteApi.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
influxdb_client_3/write_client/client/write_api.py |
Major refactor of write implementation to use a new REST client and custom request building/serialization paths. |
influxdb_client_3/write_client/_sync/rest_client.py |
Adds a new urllib3-based synchronous REST client abstraction used by WriteApi. |
influxdb_client_3/__init__.py |
Wires new write API constructor parameters (auth/gzip/base_url) and updates public kwargs documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2b26e5c to
7d74438
Compare
52cee20 to
a7076f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:251
WriteApi.write()takes parameters in(bucket, org, record, ...)order, but this test passes(org, bucket, record). This makes the test less representative of real usage and can mask issues in how query parameters are constructed.
tests/test_write_api.py:293- This call passes
(org, bucket, record)instead of(bucket, org, record), which makes the timeout test less representative of real usage and inconsistent with other tests in this file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
influxdb_client_3/write_client/client/write_api.py:1171
__getstate__deletes_write_service, butWriteApino longer defines that attribute. This will raiseKeyErrorduring pickling (anddel state['_subject']/del state['_disposable']can also KeyError depending on instance state). Usestate.pop(..., None)and remove the stale_write_servicereference.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
influxdb_client_3/write_client/client/write_api.py:1181
__setstate__callsself.__init__(self._write_options, self._point_settings, ...), butWriteApi.__init__requirestoken,bucket, andorgas the first arguments. As written, unpickling will raiseTypeError. Recreate the Rx batching pipeline based on the restored_write_optionsinstead of re-calling__init__with the wrong signature.
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
WriteApi.write()expects positional arguments in the order(bucket, org, record). This call passes org first, which swaps the query parameters and makes the test less representative of real usage. Use keyword arguments (or correct positional order) to avoid accidental swaps.
tests/test_write_api.py:293WriteApi.write()positional argument order is(bucket, org, record). This test passes org then bucket, which is easy to get wrong and can hide routing/query-param bugs. Prefer keyword arguments to make the intent explicit (and keep_request_timeoutbehavior under test).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
tests/test_write_api.py:29
mock_urllib3_timeout_requestis used as aside_effectforurllib3._request_methods.RequestMethods.request, which passes theRequestMethodsinstance as the first positional arg. The helper currently omits that parameter, somethod/urlare shifted and the test can behave incorrectly.
tests/test_write_api.py:252WriteApi.write()takesbucketas the first positional argument andorgas the second. This call passes them in the opposite order, so the request routing/query params will be wrong.
tests/test_write_api.py:293WriteApi.write()takesbucketas the first positional argument andorgas the second. This call passes them in the opposite order, so it won't exercise the intended timeout behavior for the right endpoint params.
influxdb_client_3/write_client/client/write_api.py:1171__getstate__deletesstate['_write_service'], but this class no longer defines_write_service. Pickling will raiseKeyErrorhere.
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
influxdb_client_3/write_client/client/write_api.py:1181
__setstate__callsself.__init__(self._write_options, self._point_settings, ...), but__init__now requirestoken,bucket,org, and other constructor args. This will break unpickling; it should just recreate the Rx batching pipeline based on the restored_write_options.
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/test_write_api.py:252
WriteApi.write()takes positional arguments in the order(bucket, org, record). This call passes org first and bucket second, which swaps routing parameters and can hide bugs in header/query handling. Prefer keyword arguments (or swap the positionals) to make the intent unambiguous.
tests/test_write_api.py:293- Same positional-argument issue as above: this passes org first and bucket second to
WriteApi.write(bucket, org, record), which inverts query parameters. Using keywords also makes the_request_timeoutintent clearer.
influxdb_client_3/write_client/client/write_api.py:1181 __getstate__/__setstate__are broken after the refactor:_write_serviceis no longer an attribute, sodel state['_write_service']will raiseKeyErrorduring pickling. Additionally,__setstate__calls__init__with the wrong arguments for the new constructor signature (it no longer accepts justwrite_optionsandpoint_settings). This makes pickling/unpickling unusable.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
influxdb_client_3/write_client/client/write_api.py:1181
__getstate__deletesstate['_write_service'], butWriteApino longer defines_write_service, so pickling will raiseKeyError. Additionally,__setstate__calls__init__with the wrong signature (it now requirestoken,bucket,org, etc.), so unpickling will fail even if__getstate__succeeds. Safer approach:pop()optional fields and recreate the Rx pipeline based on_write_optionswithout re-calling__init__.
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
WriteApi.write()expects positional arguments as(bucket, org, record, ...), but this test passes them as(org, bucket, record). That swaps query params and makes the test exercise the wrong behavior. Use keyword args (or correct positional order) to match thewrite()signature.
tests/test_write_api.py:294- This
write()call also swaps(org, bucket)positional order. Since the assertion is about timeout propagation rather than parameter order, using keyword args avoids accidentally testing the wrong thing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:252
- In this test,
WriteApi.write()expects positional args in the order(bucket, org, record). Passing org first swaps the routing parameters and can hide bugs in request construction/error handling.
tests/test_write_api.py:293 WriteApi.write()positional parameters are(bucket, org, record). This call passes org first, which makes the test exercise the wrong request routing.
87ccdef to
273b4ce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
influxdb_client_3/write_client/client/write_api.py:1179
__getstate__()deletes_write_service, butWriteApino longer sets this attribute, so pickling will raiseKeyError. Additionally,__setstate__()calls__init__()with the wrong signature (it now requirestoken/bucket/org/...).
def __getstate__(self):
"""Return a dict of attributes that you want to pickle."""
state = self.__dict__.copy()
# Remove rx
del state['_subject']
del state['_disposable']
del state['_write_service']
return state
def __setstate__(self, state):
"""Set your object with the provided dict."""
self.__dict__.update(state)
# Init Rx
self.__init__(self._write_options,
self._point_settings,
success_callback=self._success_callback,
error_callback=self._error_callback,
retry_callback=self._retry_callback)
tests/test_write_api.py:252
- Argument order is swapped in this
write()call:WriteApi.write(bucket, org, record, ...)expects bucket first, org second, but this passes org then bucket. This makes the test less meaningful and can hide real regressions.
tests/test_write_api.py:293 - Argument order is swapped in this
write()call:WriteApi.write(bucket, org, record, ...)expects bucket first, org second. As written, the test exercises the wrong parameter mapping (and still "works" only because values are strings).
| local_var_params, path, path_params, query_params, header_params, body_params = \ | ||
| self._post_write_prepare(org, bucket, body, self.default_header, **kwargs) # noqa: E501 | ||
| use_v2_api = local_var_params['use_v2_api'] | ||
|
|
||
| try: | ||
| return await self.call_api( | ||
| resource_path=path, | ||
| method='POST', | ||
| query_params=query_params, | ||
| header_params=header_params, | ||
| body=body, | ||
| async_req=local_var_params.get('async_req'), | ||
| _request_timeout=local_var_params.get('_request_timeout'), | ||
| urlopen_kw=kwargs.get('urlopen_kw', None)) | ||
| except ApiException as e: | ||
| raise self._translate_write_exception(e, use_v2_api) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
tests/test_write_api.py:252
- The positional arguments to
WriteApi.write()are(bucket, org, record), but this call passes(org, bucket, record)which will swap the query params and likely break the test intent. Use keyword args or swap the first two positional args.
tests/test_write_api.py:294 - The positional arguments to
WriteApi.write()are(bucket, org, record), but this call passes(org, bucket, record). This makes the_request_timeoutassertion test the wrong request parameters. Use keyword args or swap the first two positional args.
| # Init Rx | ||
| self.__init__(self._influxdb_client, | ||
| self._write_options, | ||
| self.__init__(self._write_options, | ||
| self._point_settings, | ||
| success_callback=self._success_callback, | ||
| error_callback=self._error_callback, |
| all_params = ['org', 'bucket', 'body', 'zap_trace_span', 'content_encoding', 'content_type', 'content_length', | ||
| 'accept', 'org_id', 'precision', 'no_sync', 'accept_partial', 'use_v2_api'] # noqa: E501 |
| if 'org_id' in local_var_params: | ||
| query_params.append(('orgID', local_var_params['org_id'])) # noqa: E501 |
| Write time-series data into InfluxDB. | ||
|
|
| import multiprocessing | ||
|
|
||
| from influxdb_client_3.write_client import InfluxDBClient, WriteOptions | ||
| from influxdb_client_3.write_client import WriteOptions |
| try: | ||
| return await self.call_api( | ||
| resource_path=path, | ||
| method='POST', | ||
| query_params=query_params, | ||
| header_params=header_params, | ||
| body=body, | ||
| async_req=local_var_params.get('async_req'), | ||
| _request_timeout=local_var_params.get('_request_timeout'), | ||
| urlopen_kw=kwargs.get('urlopen_kw', None)) |
95ddca9 to
8aee2b8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
- Coverage 77.65% 75.60% -2.06%
==========================================
Files 38 36 -2
Lines 2721 2607 -114
==========================================
- Hits 2113 1971 -142
- Misses 608 636 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes #
Proposed Changes
Remove features:
Checklist