fix: handle 201 response from write endpoint #1044
Conversation
jacobmarble
force-pushed
the
jgm-write-201
branch
from
7deef74 to
dae97e1
Compare
InfluxDB v3 returns 201 for partial write errors.
jacobmarble
force-pushed
the
jgm-write-201
branch
from
dae97e1 to
0d87652
Compare
| // older implementations of transport do not report status code | ||
| if (responseStatusCode == 204 || responseStatusCode == undefined) { | ||
| if ( |
There was a problem hiding this comment.
Is it worth handling all 2XX status codes here, in case this changes in the future?
There was a problem hiding this comment.
Yes! But! Read this and tell me what you think:
#263
There was a problem hiding this comment.
My suggestion is to modify the behavior of the clients as follows: v2 clients will accept 204 and 201 HTTP status codes, and v3 clients will accept any 2xx status codes. I've updated our backlog item accordingly and included a link to #263 - https://github.com/orgs/influxdata/projects/108/views/1?pane=issue&itemId=73988042.
There was a problem hiding this comment.
We'll also want to make the response body of a 201 partial write accessible to the caller, whether through a log or some other way I don't have a preference. 👍
There was a problem hiding this comment.
@bednar I would raise a PR to log a warning on 201 myself, but I can't see where the response body is bound/accessible in this file.
There was a problem hiding this comment.
Grepping the repo for "partial", it documents the 400 service behavior, but doesn't handle it. Since the SaaS products will soon return 400 instead of 201, I thought it isn't worth the effort to do anything more than this PR.
There was a problem hiding this comment.
@savage-engineer, the WriteApiImpl does not operate with the response HTTP body. To access the response, you should add a next callback
There was a problem hiding this comment.
Ah I see, if I understand correctly the user of the client API library is able to do that?
If that's the case then I'm happy! 👌
There was a problem hiding this comment.
No, if there is an error from InfluxDB, the client parses the "error state" from the response body and propagates it as an Error.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1044 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 35 35
Lines 1449 1450 +1
Branches 344 345 +1
=========================================
+ Hits 1449 1450 +1 ☔ View full report in Codecov by Sentry. |
InfluxDB v3 returns 201 for partial write errors.
Helps internal issue https://github.com/influxdata/idpe/issues/18710
Proposed Changes
Briefly describe your proposed changes:
Checklist
yarn testcompletes successfully