Skip to content

use Number instead of long for response context#8342

Merged
gianm merged 4 commits intoapache:masterfrom
clintropolis:response-context-longs-fix
Aug 21, 2019
Merged

use Number instead of long for response context#8342
gianm merged 4 commits intoapache:masterfrom
clintropolis:response-context-longs-fix

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

This PR fixes an issue with scan queries introduced by #8157, which I believe is caused by variation in JSON serde of the response context, and switches NUM_SCANNED_ROWS and CPU_CONSUMED_NANOS to cast to Number and use Number.longValue instead of casting to long primitive, to avoid errors of the form:

2019-08-19T22:56:36,180 WARN [HttpClient-Netty-Worker-3] org.apache.druid.java.util.http.client.NettyHttpClient - [POST http://localhost:8084/druid/v2/] Exception thrown while processing message, closing channel.
java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Long
    at org.apache.druid.query.context.ResponseContext$Key.lambda$static$3(ResponseContext.java:153) ~[classes/:?]
    at java.util.concurrent.ConcurrentHashMap.merge(ConcurrentHashMap.java:1990) ~[?:1.8.0_192]
    at org.apache.druid.query.context.ResponseContext.add(ResponseContext.java:308) ~[classes/:?]
    at org.apache.druid.query.context.ResponseContext.lambda$merge$2(ResponseContext.java:319) ~[classes/:?]
    at java.util.HashMap.forEach(HashMap.java:1289) ~[?:1.8.0_192]
    at org.apache.druid.query.context.ResponseContext.merge(ResponseContext.java:317) ~[classes/:?]
    at org.apache.druid.client.DirectDruidClient$1.handleResponse(DirectDruidClient.java:233) ~[classes/:?]
    at org.apache.druid.java.util.http.client.NettyHttpClient$1.messageReceived(NettyHttpClient.java:224) [classes/:?]
    at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70) [netty-3.10.6.Final.jar:?]
    at org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564) [netty-3.10.6.Final.jar:?]
    at org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791) [netty-3.10.6.Final.jar:?]

which result eventually in the broker failing to return a result:

org.apache.druid.query.QueryInterruptedException: query[6cc5653a-c9b8-49e3-ad9c-94c14a4a07af] url[http://localhost:8084/druid/v2/] timed out or max bytes limit reached.

This issue is repeatable by having a broker and 2 historical servers with the example wikipedia batch dataset, and issuing a SELECT * FROM "wikipedia".

After this fix the query successfully returns results.


This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster running on my laptop.

@clintropolis clintropolis added this to the 0.16.0 milestone Aug 19, 2019
@ccaominh
Copy link
Copy Markdown
Contributor

Is it possible to add a test?

Assert.assertEquals("string-value", ctx.get(ResponseContext.Key.ETAG));
Assert.assertEquals(100, ctx.get(ResponseContext.Key.NUM_SCANNED_ROWS));
ctx.add(ResponseContext.Key.NUM_SCANNED_ROWS, 10L);
Assert.assertEquals(110L, ctx.get(ResponseContext.Key.NUM_SCANNED_ROWS));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a similar assert for CPU_CONSUMED_NANOS?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the code is equivalent making a test sort of redundant, but yes anything is possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's still useful as a regression test, IMO.

Copy link
Copy Markdown
Contributor

@ccaominh ccaominh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

@gianm gianm merged commit c87b68d into apache:master Aug 21, 2019
@clintropolis clintropolis deleted the response-context-longs-fix branch August 21, 2019 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants