introduce /druid/v3 query endpoint that gives query responseContext #3319

himanshug · 2016-08-03T19:02:22Z

…in response json instead of HTTP header ("X-Druid-Response-Context")

this is primarily required to handle concerns regarding limits to the size of HTTP headers [in jetty and other places]. Currently we have a hack in place to always truncate context ( #2331 ) which makes responseContext somewhat unusable on the client side.

so, to ensure backward compatibility, this PR introduces a new query endpoint "/druid/v3" that has following response format...

{
"result": [ result1, result2,..... ],
"context": { "k1": < value > , .... }
}

Note that backward compatibility can also be achieved by utilizing a new value for HTTP "Accept" header instead of a new endpoint. But, chose new endpoint for "simplicity" (or at least thought it was simpler). I am willing to switch to that if others believe that is better than a new endpoint.

drcrallen · 2016-08-03T19:09:22Z

server/src/main/java/io/druid/server/QueryResourceV3.java

+                  );
+
+                  os.flush(); // Some types of OutputStream suppress flush errors in the .close() method.
+                  os.close();


can this be in a try with resources block to guarantee it is closed?

sure, copy/pasta from base class code... not sure what happens if the close is not called in an exception situation. will put additional try-catch in both classes .... or may be refactor a bit more so that this part is also re-used.

only thing different in this class really is not dumping context header and passing it to jsonWriter instead.

@drcrallen updated with a try-finally

nishantmonu51 · 2016-08-05T06:19:43Z

services/src/main/java/io/druid/cli/CliBroker.java

@@ -102,6 +101,7 @@ public void configure(Binder binder)

            binder.bind(JettyServerInitializer.class).to(QueryJettyServerInitializer.class).in(LazySingleton.class);
            Jerseys.addResource(binder, QueryResource.class);
+            Jerseys.addResource(binder, QueryResourceV3.class);


I believe this needs to be registered for router, historical, RT Index task and RT Nodes as well ?

@nishantmonu51 this PR is for only enabling the endpoint on broker.

full historical and realtime node support is being added in #3323

i have to admit that I had forgotten about the router, adding router in #3323 as well. (i guess router will be simpler than historical/realtime because router does not deserialize the response and just a pass through )

drcrallen · 2016-08-05T16:21:14Z

server/src/main/java/io/druid/server/QueryResourceV3.java

+ *  KIND, either express or implied. See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ * /


license looks weird, can you re-apply?

updated, not sure why my IDE license header got messed up.... it seems i need to re-apply in other PRs as well :(

xvrl · 2016-08-08T23:04:35Z

@himanshug since we're updating the API, I wonder if it would make sense to define whether we always return the results before context. This would allow us to stream results back, as well as include some query stats (or other data that we only know after the fact) as part of the context.

gianm · 2016-08-08T23:09:08Z

server/src/test/java/io/druid/server/QueryResourceV3Test.java

+    try {
+      QueryResourceV3.serializeResponseToOS(
+          TestHelper.getObjectMapper().writer(),
+          new ByteArrayOutputStream(),


The thing I was concerned about was if the serializing fails before Jackson gets to the Yielder. This test wouldn't verify that (although it does verify a separate equally important thing).

Could you also do a test where the Yielder is just a normal Yielder (perhaps from a ResourceClosingSequence), but the output stream fails on all calls to write? I think that'd be enough to smoke out the case I'm thinking of.

added another test to verify OutputStream failure, yielder is closed as expected.

himanshug · 2016-08-09T20:00:06Z

@xvrl we discussed the ordering or result and context in #3319 (diff)
updated the querying documentation with information about new endpoint. I am marking it experimental for now so as to be able to change things in near future if a problem is discovered with the response format proposed here.

…in response json instead of HTTP header

himanshug · 2016-08-11T16:44:44Z

moved to 0.9.3 to unblock 0.9.2

fjy · 2016-11-08T22:50:59Z

@himanshug there's some merge conflicts

himanshug · 2016-11-09T17:36:36Z

after some more thought on this recently, I'm planning to have both pre and post context, and, make the response format be...

{
"pre-context" : { "k": < value > , .... }
"result": [ result1, result2,..... ],
"post-context": { "k1": < value > , .... }
}

that can enable the cases where broker sends a query to some historical which does not have all the segments that broker thought it has (due to zookeeper+curator weirdness e.g.). in those cases, query fails currently.
we could envision a change where historical could quickly stream a pre-context with the list of segments it does not have and broker could try other historicals for those segments... in the end query would not have to fail.

stale · 2019-02-28T06:15:02Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2019-03-07T07:09:32Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

himanshug added the Discuss label Aug 3, 2016

drcrallen reviewed Aug 3, 2016
View reviewed changes

himanshug force-pushed the context_in_resp branch from 4a32194 to 41ac1b0 Compare August 4, 2016 03:53

himanshug added Improvement Release Notes and removed Discuss labels Aug 4, 2016

himanshug added this to the 0.9.2 milestone Aug 4, 2016

This was referenced Aug 4, 2016

optionally dump list of segment-ids used to process the query #3322

Closed

WIP: optionally configure DirectDruidClient to use /druid/v3 instead of /druid/v2 #3323

Closed

[Proposal] dump query processing performance metrics from various stages #3324

Closed

nishantmonu51 reviewed Aug 5, 2016
View reviewed changes

himanshug force-pushed the context_in_resp branch from 41ac1b0 to 651e823 Compare August 5, 2016 14:40

drcrallen reviewed Aug 5, 2016
View reviewed changes

himanshug force-pushed the context_in_resp branch from 651e823 to b9be2d8 Compare August 5, 2016 21:32

gianm reviewed Aug 8, 2016
View reviewed changes

himanshug force-pushed the context_in_resp branch 2 times, most recently from 2b8a8c0 to 9637a6d Compare August 9, 2016 19:58

himanshug force-pushed the context_in_resp branch from 9637a6d to d61cc54 Compare August 9, 2016 20:00

introduce /druid/v3 query endpoint that gives query response context …

53d7cee

…in response json instead of HTTP header

himanshug force-pushed the context_in_resp branch from d61cc54 to 53d7cee Compare August 9, 2016 20:02

himanshug modified the milestones: 0.9.3, 0.9.2 Aug 11, 2016

himanshug mentioned this pull request Aug 17, 2016

WIP: dump query processing performance metrics from various stages #3371

Closed

himanshug added the Discuss label Nov 9, 2016

himanshug removed this from the 0.9.3 milestone Nov 11, 2016

gianm mentioned this pull request Dec 28, 2016

Concurrent modification of responseContext in QueryRunners #3803

Closed

himanshug mentioned this pull request Dec 12, 2017

RetryQueryRunner might fail on parsing responseContext to get missingSegments #5153

Closed

stale bot added the stale label Feb 28, 2019

stale bot closed this Mar 7, 2019

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3319 Provide physical plan for SQL explain

aa818d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce /druid/v3 query endpoint that gives query responseContext #3319

introduce /druid/v3 query endpoint that gives query responseContext #3319

himanshug commented Aug 3, 2016 •

edited

drcrallen Aug 3, 2016

himanshug Aug 3, 2016

himanshug Aug 4, 2016

nishantmonu51 Aug 5, 2016

himanshug Aug 5, 2016

drcrallen Aug 5, 2016

himanshug Aug 5, 2016

xvrl commented Aug 8, 2016

gianm Aug 8, 2016

himanshug Aug 9, 2016

himanshug commented Aug 9, 2016

himanshug commented Aug 11, 2016

fjy commented Nov 8, 2016

himanshug commented Nov 9, 2016

stale bot commented Feb 28, 2019

stale bot commented Mar 7, 2019

introduce /druid/v3 query endpoint that gives query responseContext #3319

introduce /druid/v3 query endpoint that gives query responseContext #3319

Conversation

himanshug commented Aug 3, 2016 • edited

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

xvrl commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Aug 9, 2016

himanshug commented Aug 11, 2016

fjy commented Nov 8, 2016

himanshug commented Nov 9, 2016

stale bot commented Feb 28, 2019

stale bot commented Mar 7, 2019

himanshug commented Aug 3, 2016 •

edited