-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
introduce /druid/v3 query endpoint that gives query responseContext #3319
Conversation
); | ||
|
||
os.flush(); // Some types of OutputStream suppress flush errors in the .close() method. | ||
os.close(); |
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.
can this be in a try with resources block to guarantee it is closed?
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.
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.
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.
@drcrallen updated with a try-finally
4a32194
to
41ac1b0
Compare
@@ -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); |
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.
I believe this needs to be registered for router, historical, RT Index task and RT Nodes as well ?
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.
@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 )
41ac1b0
to
651e823
Compare
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
* / |
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.
license looks weird, can you re-apply?
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.
updated, not sure why my IDE license header got messed up.... it seems i need to re-apply in other PRs as well :(
651e823
to
b9be2d8
Compare
@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. |
try { | ||
QueryResourceV3.serializeResponseToOS( | ||
TestHelper.getObjectMapper().writer(), | ||
new ByteArrayOutputStream(), |
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.
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.
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.
added another test to verify OutputStream failure, yielder is closed as expected.
2b8a8c0
to
9637a6d
Compare
@xvrl we discussed the ordering or result and context in #3319 (diff) |
9637a6d
to
d61cc54
Compare
…in response json instead of HTTP header
d61cc54
to
53d7cee
Compare
moved to 0.9.3 to unblock 0.9.2 |
@himanshug there's some merge conflicts |
after some more thought on this recently, I'm planning to have both pre and post context, and, make the response format be...
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. |
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. |
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. |
…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.