fix: partial write error handling - #239
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors write error handling to better distinguish partial-write errors from generic write failures by parsing JSON error bodies and conditionally raising InfluxDBPartialWriteError only when accept_partial is enabled and the response matches the expected “rejected rows” list format.
Changes:
- Update write exception translation to parse JSON bodies and format per-line rejection details for partial writes.
- Adjust sync REST client response decoding to normalize empty bodies to
None. - Expand and update tests to cover partial-write detection, message formatting, and fallback behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_write_api.py |
Adds broader unit coverage for write-error classification and fallback messaging; updates existing error-translation tests. |
tests/test_influxdb_client_3_integration.py |
Loosens assertion on v3 error message content and narrows the tested accept_partial configuration. |
influxdb_client_3/write_client/client/write_api.py |
Implements new JSON parsing and partial-write detection/formatting in _translate_write_exception. |
influxdb_client_3/write_client/_sync/rest_client.py |
Avoids decoding empty bodies and normalizes empty data to None. |
influxdb_client_3/exceptions/exceptions.py |
Changes exception message handling and updates partial-write error types/constructors. |
Suppressed comments (1)
influxdb_client_3/exceptions/exceptions.py:69
- These PEP 604 union annotations (
int | None, etc.) will fail to parse on Python <3.10. UseOptional[...](withOptionalimported fromtyping) to preserve compatibility.
class InfluxDBPartialWriteLineError:
line_number: int | None
error_message: str | None
original_line: str | None
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def __init__(self, response: HTTPResponse = None, message: str = None): | ||
| """Initialize the InfluxDBError handler.""" | ||
| if response is not None: | ||
| self.response = response | ||
| self.message = self._get_message(response) | ||
| self.message = message | ||
| self.retry_after = response.getheader('Retry-After') | ||
| else: | ||
| self.response = None | ||
| self.message = message or 'no response' | ||
| self.retry_after = None | ||
| super().__init__(self.message) |
0e798b1 to
3352e90
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
influxdb_client_3/exceptions/exceptions.py:5
- Python 3.9 is still in the CI matrix, but this module now uses PEP 604 union syntax elsewhere (
int | None), so you’ll needOptionalavailable for 3.9-compatible type annotations.
from typing import List
influxdb_client_3/exceptions/exceptions.py:69
int | None/str | Nonetype syntax is a Python 3.10+ feature and will raise a SyntaxError on Python 3.9 (which is still in.github/workflows/pylint.yml). UseOptional[...]instead for compatibility.
class InfluxDBPartialWriteLineError:
line_number: int | None
error_message: str | None
original_line: str | None
| def __init__(self, response: HTTPResponse = None, message: str = None): | ||
| """Initialize the InfluxDBError handler.""" | ||
| if response is not None: | ||
| self.response = response | ||
| self.message = self._get_message(response) | ||
| self.message = message |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
- Coverage 86.90% 86.73% -0.17%
==========================================
Files 28 28
Lines 2084 2050 -34
==========================================
- Hits 1811 1778 -33
+ Misses 273 272 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1ea4183 to
1c5111d
Compare
NguyenHoangSon96
changed the title
1c5111d to
28581e6
Compare
28581e6 to
4f769ef
Compare
bednar
left a comment
bednar
left a comment
There was a problem hiding this comment.
I found five issues that need to be addressed before approval.
| @staticmethod | ||
| def _extract_fallback_reason(response) -> str: | ||
| # Fallback to header | ||
| for header_key in ["X-Platform-Error-Code", "X-Influx-Error", "X-InfluxDb-Error"]: |
There was a problem hiding this comment.
[P1] _translate_write_exception reaches this fallback for ApiException(status=0, reason=...), which is how RestClient.request reports SSL/transport failures. Those exceptions have response=None, so this dereference replaces the original transport error with AttributeError. Handle the no-response case and preserve the original exception/reason.
| if response is not None: | ||
| self.response = response | ||
| self.message = self._get_message(response) | ||
| self.message = message |
There was a problem hiding this comment.
[P1] WritesRetry.increment() constructs InfluxDBError(response=response) before WriteApi translates the error. With this assignment, every retry callback and warning receives message=None (for example, Reason: 'None'). Please preserve a meaningful message for this path, either here or by moving the retry-path parsing to an appropriate caller.
| return (status_code == HTTPStatus.BAD_REQUEST and | ||
| accept_partial is True and | ||
| use_v2_api is False and | ||
| (isinstance(root.get('data'), list) and len(root.get('data')) > 0) |
There was a problem hiding this comment.
[P1] A non-empty data array alone is not enough to identify a partial write. A generic 400 response with a message and an array-valued data field is currently raised as InfluxDBPartialWriteError, even though it does not have the documented partial-write shape. Require the expected error field/payload shape and add a regression test for this case.
|
|
||
| def __init__(self, response: HTTPResponse, line_errors: List[InfluxDBPartialWriteLineError]): | ||
| super().__init__(response=response) | ||
| def __init__(self, response: HTTPResponse, message: str, line_errors: List[InfluxDBPartialWriteLineError]): |
There was a problem hiding this comment.
[P2] This changes the public constructor and removes the public InfluxDBPartialWriteError.from_response() classmethod. The exception is exported from influxdb_client_3.exceptions, so existing consumers can now fail with TypeError or AttributeError. Please retain a backward-compatible entry point, or explicitly document this as a breaking API change.
| ### Bug Fixes | ||
|
|
||
| 1. [#239](https://github.com/InfluxCommunity/influxdb3-python/pull/239): | ||
| - Only throws `InfluxDBPartialWriteException` when: |
There was a problem hiding this comment.
[P2] The documented exception name is incorrect: the public class is InfluxDBPartialWriteError, not InfluxDBPartialWriteException.
Closes #
Proposed Changes
The current exception classes hierarchy are
InfluxDBError->InfluxDBPartialWriteErrorandApiExceptionInfluxDBErrorthere is an important function_get_message(self, response), this function will run everytime a subclass ofInfluxDBErroris initialized, inside this function there is a quite heavy function will run, that is_parse_partial_write_line_error_info(data), so when a partial write error occurs_get_message(self, response)will be called at least twice.Some issues:
InfluxDBErrorwill have the same way of parsing the error messages like we do in_get_message(self, response)function is not correct... I think.We can try to make everything work more efficiently as possible with the current exception classes implementation, but I still feel It very wrong on the architecture and design perspective.
What I'm trying to do right now is moving all logic inside exception classes to WriteApi class (or wherever than inside exception classes) and making them as lightweight as possible. They should only carry information about the errors;
Checklist