Skip to content

Timeout and maxScatterGatherBytes handling for queries run by Druid SQL#4305

Merged
gianm merged 4 commits intoapache:masterfrom
jon-wei:sql_timeout
May 23, 2017
Merged

Timeout and maxScatterGatherBytes handling for queries run by Druid SQL#4305
gianm merged 4 commits intoapache:masterfrom
jon-wei:sql_timeout

Conversation

@jon-wei
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei commented May 20, 2017

This PR sets timeout and maxScatterGatherBytes parameters (#4229) in the query context/responseContext for SQL-originated queries.

Without this, SQL queries fail with a NullPointerException at this line in DirectDruidClient.run():

      long timeoutAt = ((Long) context.get(QUERY_FAIL_TIME)).longValue();

@jon-wei jon-wei added the Bug label May 20, 2017
@gianm gianm added this to the 0.10.1 milestone May 20, 2017
morePages.set(false);
final AtomicBoolean gotResult = new AtomicBoolean();

queryWithPagination = (SelectQuery) QueryMaker.withDefaultTimeoutAndMaxScatterGatherBytes(
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.

IMO, non-finals that are redefined halfway through a method make things difficult to follow. Why not apply this to queryWithPagination directly when it's created?


public static Map<String, Object> makeResponseContextForQuery(Query query)
{
final Map<String, Object> responseContext = new MapMaker().makeMap();
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 looks like this is just making a new ConcurrentHashMap, might as well create it directly rather than going through this (somewhat obscure?) helper.

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.

Also, could this code be shared with QueryResource through a common helper -- maybe a helper located in DirectDruidClient, which seems to be what cares about the values? It looks a bit finicky to be a good idea to copy and paste it.

@jon-wei
Copy link
Copy Markdown
Contributor Author

jon-wei commented May 23, 2017

@gianm Addressed your comments

@gianm
Copy link
Copy Markdown
Contributor

gianm commented May 23, 2017

@jon-wei, compilation issue:

/home/travis/build/druid-io/druid/sql/src/main/java/io/druid/sql/calcite/schema/DruidSchema.java:317: error: cannot find symbol
                                                            QueryMaker.makeResponseContextForQuery(segmentMetadataQuery)
                                                                      ^

@jon-wei
Copy link
Copy Markdown
Contributor Author

jon-wei commented May 23, 2017

@gianm my bad, fixed

@gianm
Copy link
Copy Markdown
Contributor

gianm commented May 23, 2017

@jon-wei thanks; it looks like there's a different error now, from checkstyle.

@gianm gianm merged commit d49e53e into apache:master May 23, 2017
@jon-wei jon-wei deleted the sql_timeout branch October 6, 2017 22:18
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.

2 participants