Introduce syntax for marking a partial response with chunking #7368

jsternberg · 2016-09-27T16:36:22Z

The partial tag has been added to the JSON response of a series and
the result so that a client knows when more of the series or result will
be sent in a future JSON chunk.

This helps interactive clients who don't want to wait for all of the
data to know if it is done processing the current series or the current
result. Previously, the client had to guess if the next chunk would
refer to the same result or a new result and it had to match the name
and tags of the two series to know if they were the same series. Now,
the client just needs to check the partial field included with the
response to know if it should expect more.

mention-bot · 2016-09-27T16:36:24Z

@jsternberg, thanks for your PR! By analyzing the annotation information on this pull request, we identified @e-dard, @gunnaraasen and @joelegasse to be potential reviewers

jwilder

LGTM. @joelegasse @corylanou Can you one of you take a look as well?

jwilder · 2016-10-26T02:14:10Z

--- FAIL: TestServer_Query_ImplicitEndTime (2.01s)
    server_test.go:7039: aggregate query: unexpected results
        query:  SELECT mean(value) FROM cpu WHERE time > now() - 1m GROUP BY time(1m) FILL(none)
        params:  map[db:[db0]]
        exp:    {"results":[{"statement_id":0,"series":[{"name":"cpu","columns":["time","mean"],"values":[["2016-10-25T21:33:00Z",1]]}]}]}
        actual: {"results":[{"statement_id":0,"series":[{"name":"cpu","columns":["time","mean"],"values":[["2016-10-25T21:32:00Z",1]]}]}]}

jsternberg · 2016-10-26T02:47:42Z

Ah crap. It's a flaky test from something I just merged. I'll fix it.

jsternberg · 2016-10-26T14:25:06Z

I think I fixed the flaky test.

corylanou

Only concern is on the changing of the result struct. Otherwise looks good.

corylanou · 2016-10-25T14:50:58Z

cmd/influxd/run/server_suite_test.go

@@ -19,13 +19,13 @@ func init() {
 			&Query{
 				name:    "create database should succeed",
 				command: `CREATE DATABASE db0`,
-				exp:     `{"results":[{}]}`,
+				exp:     `{"results":[{"statement_id":0}]}`,


This feels like a pretty big breaking change. I imagine a lot of scripts are written to expect the empty result. I'm not sure how we would be affecting the user by adding this new field in the actual result itself.

Since the statement_id is a generated ID, and doesn't matter if there are no results, the field should probably have omitempty added to the struct tag as well. That would preserve the existing "empty object" result, and still allow for tracking partial series results

No, the statement_id is pretty important. It tells the client which statement that empty response is for so it knows it ran. Otherwise, there's no way to know which statement that empty response references.

Statement ID's would have to start at 1, not 0 then, as 0 can be a valid statement ID it looks like. I would agree that I would like to see it omitted as it creates a lot of noise when there is no response to be had.

Why is statement ID important for an empty result? I don't understand the logic.

It tells the client that the statement was executed. How would you know what the empty response was for without the statement id?

If it didn't execute you get an error. That's how it works today right?

Yes, you would. But the point of this PR is to also give you information about what is being run interactively for chunking.

A different question. What benefit is there for statement ID being omitted from these results? It seems like an unnecessary complication to omit the statement id specifically for those results. Clients are supposed to check for error, not a blank JSON response. It looks like influxdb-python checks for the error field.

jsternberg · 2016-10-26T16:20:15Z

/cc @aviau, @toddboom, and @dmke since these are the primary maintainers behind influxdb-python and influxdb-ruby and will be the ones most affected by this change.

Can you confirm if this change will be fine and if it will help in the future for client library development? Thanks.

dmke · 2016-10-26T20:51:05Z

@jsternberg I believe the Ruby gem can handle this without any modification. Can you verify that a "statement_id" will be included in every (successful) response from the server?

Currently, the high level methods only care about json["results"][i]["series"] values, while
exceptions are generated from json["results"][i]["error"] values; other keys in json["results"][i] are silently ignored.

Some user-facing methods however return the raw HTTP response object which could cause trouble if the user has code validating that the response only contains "series" and/or "error" keys.

I have added some statement ids into the test stubs and the suite still passes (see Travis and diff).

jsternberg · 2016-10-26T20:52:56Z

Thanks. Yes, if we merge this, statement_id will be included in every result. I also noticed in your diff you have a typo for statement_id.

dmke · 2016-10-26T20:59:25Z

👍

The `partial` tag has been added to the JSON response of a series and the result so that a client knows when more of the series or result will be sent in a future JSON chunk. This helps interactive clients who don't want to wait for all of the data to know if it is done processing the current series or the current result. Previously, the client had to guess if the next chunk would refer to the same result or a new result and it had to match the name and tags of the two series to know if they were the same series. Now, the client just needs to check the `partial` field included with the response to know if it should expect more. Fixed `max-row-limit` so it counts rows instead of results and it truncates the response when the `max-row-limit` is reached.

These seem to have been broken in #7368.

max-row-limit was set at 10000 since 1.0, but due to a bug it was effectively 0 (disabled). 1.2 fixed this bug via #7368, but this caused a breaking change w/ Grafana and any users upgrading from <1.2 who had not disabled the config manually.

jsternberg force-pushed the js-partial-in-json-response branch 3 times, most recently from deb8dfe to 79c5a3b Compare September 30, 2016 17:27

jsternberg force-pushed the js-partial-in-json-response branch 2 times, most recently from 706b504 to 871bb3f Compare October 25, 2016 15:51

jwilder added this to the 1.1.0 milestone Oct 25, 2016

jwilder reviewed Oct 25, 2016

View reviewed changes

jsternberg force-pushed the js-partial-in-json-response branch from 871bb3f to aa84c08 Compare October 25, 2016 21:26

jsternberg force-pushed the js-partial-in-json-response branch from aa84c08 to 9c7b4a5 Compare October 26, 2016 14:24

corylanou approved these changes Oct 26, 2016

View reviewed changes

dmke added a commit to InfluxCommunity/influxdb-ruby that referenced this pull request Oct 26, 2016

Incorporate changes from influxdata/influxdb#7368

55f06bc

dmke added a commit to InfluxCommunity/influxdb-ruby that referenced this pull request Oct 26, 2016

Incorporate changes from influxdata/influxdb#7368

a6dffdd

jwilder modified the milestones: 1.2.0, 1.1.0 Oct 27, 2016

jsternberg force-pushed the js-partial-in-json-response branch 4 times, most recently from d9f42db to 82e9192 Compare November 22, 2016 17:14

jsternberg force-pushed the js-partial-in-json-response branch from 82e9192 to b4db76c Compare November 22, 2016 17:16

jsternberg merged commit 9337025 into master Nov 29, 2016

jsternberg deleted the js-partial-in-json-response branch November 29, 2016 17:17

kostasb mentioned this pull request Dec 5, 2016

[InfluxDB] Document truncated HTTP responses influxdata/docs.influxdata.com-ARCHIVE#909

Closed

mark-rushakoff added a commit that referenced this pull request Jan 9, 2017

Fix broken server benchmarks

c126dc5

These seem to have been broken in #7368.

mark-rushakoff mentioned this pull request Jan 9, 2017

Fix broken server benchmarks #7808

Merged

2 tasks

dmke added a commit to InfluxCommunity/influxdb-ruby that referenced this pull request Jan 26, 2017

Incorporate changes from influxdata/influxdb#7368

25e1963

khenderick mentioned this pull request Jan 26, 2017

Influxdb: Support InfluxDB 1.2+ Partial data responses grafana/grafana#7380

Closed

jwilder mentioned this pull request Feb 8, 2017

[1.2.0] Show series, fields and tags with over 10k series in DB are truncated (unexpected behaviour) #7962

Closed

rkuchan mentioned this pull request Mar 1, 2017

The API reference do not reflect the changes made between 1.0 and 1.2 influxdata/docs.influxdata.com-ARCHIVE#1032

Closed

jwilder mentioned this pull request Mar 14, 2017

Disable max-row-limit by default #8133

Merged

3 tasks

dleutnant mentioned this pull request Oct 17, 2017

Error in overscope_eval_next(overscope, expr) : object 'statement_id' not found dleutnant/influxdbr#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce syntax for marking a partial response with chunking #7368

Introduce syntax for marking a partial response with chunking #7368

jsternberg commented Sep 27, 2016

mention-bot commented Sep 27, 2016

jwilder left a comment

jwilder commented Oct 26, 2016

jsternberg commented Oct 26, 2016 •

edited

Loading

jsternberg commented Oct 26, 2016

corylanou left a comment

corylanou Oct 25, 2016

joelegasse Oct 26, 2016

jsternberg Oct 26, 2016

corylanou Oct 26, 2016

corylanou Oct 26, 2016

jsternberg Oct 26, 2016

corylanou Oct 26, 2016

jsternberg Oct 26, 2016

jsternberg commented Oct 26, 2016

dmke commented Oct 26, 2016

jsternberg commented Oct 26, 2016

dmke commented Oct 26, 2016

Introduce syntax for marking a partial response with chunking #7368

Introduce syntax for marking a partial response with chunking #7368

Conversation

jsternberg commented Sep 27, 2016

mention-bot commented Sep 27, 2016

jwilder left a comment

Choose a reason for hiding this comment

jwilder commented Oct 26, 2016

jsternberg commented Oct 26, 2016 • edited Loading

jsternberg commented Oct 26, 2016

corylanou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsternberg commented Oct 26, 2016

dmke commented Oct 26, 2016

jsternberg commented Oct 26, 2016

dmke commented Oct 26, 2016

jsternberg commented Oct 26, 2016 •

edited

Loading