Skip to content

Conversation

@NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Jun 16, 2025

Closes #242

Proposed Changes

  • Add function to check InfluxDB version
    NOTE: It will call /ping api from InfluxDB v2 (branch main-2.x) and returns version 2.0 in the header of the response.
    I don't think this is the right behavior because client v3 should work with InfluxDB v3 but I don't think we can do anything from our side.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Jun 16, 2025
@codecov
Copy link

codecov bot commented Jun 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.14%. Comparing base (ac2dba9) to head (a40b3b4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
+ Coverage   90.05%   90.14%   +0.08%     
==========================================
  Files          20       20              
  Lines        1197     1207      +10     
  Branches      187      188       +1     
==========================================
+ Hits         1078     1088      +10     
  Misses         42       42              
  Partials       77       77              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find below my notes:

  • I'd call the method rather getServerVersion as that is the main use-case, the fact that we are calling ping is just implementation detail
  • I'd also allow to return I don't know the version state, as that could be also possible (so maybe the return value could be @nullable or throw specific exception for that case) + javadoc
  • the unreachable server or any network error should still throw InfluxDBApiException exception
  • the server version should be parsed not only from the header, but also from the response body

Version 2 response (in header):

 % curl -G -i https://us-east-1-1.aws.cloud2.influxdata.com/ping
HTTP/2 204 
date: Thu, 12 Jun 2025 11:27:04 GMT
trace-id: 3d99d4fd7fa9e3c4
trace-sampled: false
x-influxdb-build: cloud2
x-influxdb-version: 2.0
strict-transport-security: max-age=31536000; includeSubDomains
x-influxdb-request-id: eb997c65c40ccf3d18cc244b2d24d856
x-influxdb-build: Cloud

Version 3 response (in body):

% curl -G -i http://localhost:8181/ping --header "Authorization: Bearer apiv3_....."
HTTP/1.1 200 OK
access-control-allow-origin: *
transfer-encoding: chunked
date: Thu, 12 Jun 2025 11:29:37 GMT

{"version":"3.0.2","revision":"d80d6cd600","process_id":"c876e998-9c58-4ba6-a4d4-bcb0a7ee9d7b"}

@NguyenHoangSon96
Copy link
Contributor Author

Find below my notes:

  • I'd call the method rather getServerVersion as that is the main use-case, the fact that we are calling ping is just implementation detail
  • I'd also allow to return I don't know the version state, as that could be also possible (so maybe the return value could be @nullable or throw specific exception for that case) + javadoc
  • the unreachable server or any network error should still throw InfluxDBApiException exception
  • the server version should be parsed not only from the header, but also from the response body

Version 2 response (in header):

 % curl -G -i https://us-east-1-1.aws.cloud2.influxdata.com/ping
HTTP/2 204 
date: Thu, 12 Jun 2025 11:27:04 GMT
trace-id: 3d99d4fd7fa9e3c4
trace-sampled: false
x-influxdb-build: cloud2
x-influxdb-version: 2.0
strict-transport-security: max-age=31536000; includeSubDomains
x-influxdb-request-id: eb997c65c40ccf3d18cc244b2d24d856
x-influxdb-build: Cloud

Version 3 response (in body):

% curl -G -i http://localhost:8181/ping --header "Authorization: Bearer apiv3_....."
HTTP/1.1 200 OK
access-control-allow-origin: *
transfer-encoding: chunked
date: Thu, 12 Jun 2025 11:29:37 GMT

{"version":"3.0.2","revision":"d80d6cd600","process_id":"c876e998-9c58-4ba6-a4d4-bcb0a7ee9d7b"}

Hi, so what about InfluxDBApiHttpException. Are we still using it? .e.g: in writeData function

@jansimonb
Copy link
Contributor

Hi, so what about InfluxDBApiHttpException. Are we still using it? .e.g: in writeData function

No need to change anything in writeData, I just wanted to tell you keep the exception handling as is.

@NguyenHoangSon96
Copy link
Contributor Author

@jansimonb
Hi can you review it again
There is a lack of test case warning but I think we can skip it.

Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To increase coverage and to test valid use-cases, please, add tests where:

  1. version header is not set + non-json result is retuned
  2. version header is not set + no response body present

Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 👍

@NguyenHoangSon96 NguyenHoangSon96 merged commit a12fa64 into main Jun 19, 2025
9 checks passed
@NguyenHoangSon96 NguyenHoangSon96 deleted the feat/check-version branch June 19, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add version/ping method

3 participants