-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: POST
query variants serializes 'q'
parameter into HTTP body
#765
Conversation
Can you update the CI Test matrix with influxdb-2 as well ? |
Yes I will do. It will be a little bit tricky but I will do my best. |
Codecov Report
@@ Coverage Diff @@
## master #765 +/- ##
============================================
- Coverage 58.45% 54.11% -4.35%
+ Complexity 414 383 -31
============================================
Files 70 70
Lines 2571 2574 +3
Branches 269 268 -1
============================================
- Hits 1503 1393 -110
- Misses 1002 1116 +114
+ Partials 66 65 -1
Continue to review full report at Codecov.
|
@majst01, InfluxDB 2 compatibility endpoints requires Database and retention policy mapping for queries and writes. So there is no easy way how to use existing test cases with InfluxDB 2. I've prepared separete test case for InfluxDB 2 - InfluxDB2Test You can start this testcase by: |
query
parameter for POST variants is serialized into HTTP body
666ebb5
to
8e425fd
Compare
query
parameter for POST variants is serialized into HTTP bodyPOST
query variants serializes 'q'
parameter into HTTP body
@majst01, I had to change how is PR implemented:
Can you review this changes? Thx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, does this mean that influxdb-java is now compatible with influxdb v2 ? If yes this must be reflected in the Readme, currently it says:
Note: This library is for use with InfluxDB 1.x. For connecting to InfluxDB 2.x instances, please use the influxdb-client-java client.
Please adjust accordingly
The I think that current text is correct, because the client doesn't supports v2 API. |
Understood, but even that must be clarified in the Readme, please add a short description with a link to the official documentation. |
@majst01, note changed to |
@bednar merged, and now all builds fail ?? Could you please check why. |
@bednar it still does not compile anymore, please fix this, otherwise i have to revert this |
@majst01 currently I'am fixing that issue, stay tuned. The problem is caused by definition of GitHub PRs workflow: All 1.x builds are running agains 1.8... |
The PR with fix is prepared: #773 |
With this change, I am now seeing failures in 2.22 that I didn't see before in 2.21.
Using InfluxDB 1.8.10.
(which defaults to Is this expected? What do I need to change? |
Hi @bentatham, can you share a little bit more about your code? How do you initialise the client? How do you run your query? I just tried following code and everything works fine: try (InfluxDB client = InfluxDBFactory.connect("http://localhost:8086")) {
BoundParameterQuery query = QueryBuilder
.newQuery("SELECT * FROM cpu WHERE time >= $startTime AND time <= $endTime")
.forDatabase("my_database")
.bind("startTime", "2015-08-18T00:00:00Z")
.bind("endTime", "2015-08-18T00:30:00Z")
.create();
QueryResult result = client.query(query);
System.out.println("result = " + result);
} Regards |
@bednar Thanks for getting back to me: We are using influxDB behind vertx, but I don't think that should really matter unless something in the query object is massively not thread-safe and is consistently hitting a volatility issue or something really obscure. Initialization:
Batching is not enabled. And then invoked with:
Maybe something is different about the |
So the
I tried it with a direct connection to InfluxDB and everything works fine.
It doesn't matter. |
I'm seeing the same behavior as @bentatham I submitted issue !848 describing the problem. It seems to be related (at least for me) to enableGzip being set to true. |
Proposed Changes
The
query
parameter forpost
variants of query is serialized into HTTP body.It is usefull when you hit the
nginx
limit forRequest-URI
length, eq. HTTP error:414 Request-URI Too Large
.Following query variants also respects the
requiresPost
settings:QueryResult query(Query, TimeUnit)
void query(Query, int, BiConsumer<Cancellable, QueryResult>, Runnable, Consumer<Throwable>)
Checklist
compile-and-test.sh
completes successfully