Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable routing of SQL queries at Router #11566

Merged
merged 14 commits into from
Aug 13, 2021
Merged

Enable routing of SQL queries at Router #11566

merged 14 commits into from
Aug 13, 2021

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Aug 10, 2021 •
edited
Loading

Description

This PR adds a new property druid.router.sql.enable which allows the
Router to handle SQL queries when set to true.

This change does not affect Avatica JDBC requests and they are still routed
by hashing the Connection ID.

To allow parsing of the request object as a SqlQuery (contained in module druid-sql),
some classes have been moved from druid-server to druid-services with
the same package name.

Major Changes

  • Add property druid.router.sql.enable
  • Route SQL queries in ManualTieredBrokerSelectorStrategy based on query context
  • Move the following classes from druid-server to druid-services
    • AsyncQueryForwardingServlet
    • QueryHostFinder
    • TieredBrokerHostSelector
    • TieredBrokerSelectorStrategy and its implementations
    • RouterResource
    • CoordinatorRuleManager
    • corresponding unit tests
  • Refactor AsyncQueryForwardingServlet to use common code for routing native and SQL queries
  • Minor refactor of QueryHostFinder to re-use common code
  • Add new test dependencies in druid-services

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@kfaraz kfaraz marked this pull request as ready for review August 10, 2021 07:56
abhishekagarwal87
abhishekagarwal87 reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 left a comment

Choose a reason for hiding this comment

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

Thank you @kfaraz for the PR. Other than inline comments, can we also run tests for AsyncQueryForwardingServlet with the sql routing enabled for router?

services/src/main/java/org/apache/druid/server/router/TieredBrokerHostSelector.java Outdated Show resolved Hide resolved
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java Outdated Show resolved Hide resolved
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java Show resolved Hide resolved
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java Outdated Show resolved Hide resolved
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java Outdated Show resolved Hide resolved
abhishekagarwal87
abhishekagarwal87 approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 left a comment

Choose a reason for hiding this comment

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

minor comments. LGTM otherwise.

docs/design/router.md Show resolved Hide resolved
services/src/main/java/org/apache/druid/server/router/TieredBrokerSelectorStrategy.java Show resolved Hide resolved
maytasm
maytasm approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@maytasm maytasm left a comment

Choose a reason for hiding this comment

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

Minor comments

extensions-core/hdfs-storage/pom.xml
@@ -377,6 +377,11 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protobuf-java (required while running HdfsFirehoseFactoryTest) was initially coming as a transitive dependency through druid-server -> avatica-core
But since avatica-core has been removed from druid-server in this PR, protobuf-java has been explicitly added here.

services/src/main/java/org/apache/druid/server/router/TieredBrokerHostSelector.java
@@ -257,6 +268,42 @@ public String getDefaultServiceName()
return new Pair<>(brokerServiceName, nodesHolder.pick());
}

public Pair<String, Server> selectForSql(SqlQuery sqlQuery)
{
synchronized (lock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can part of this method be refactor to share common code with the select method?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I just realized that this block is not even required for selectForSql since ruleManager is not used to select the target server for sql query. @kfaraz - shall this be fixed?

Copy link
Contributor Author

@kfaraz kfaraz Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for catching this, @abhishekagarwal87 . We still need the block as we need to check on the started field of the TieredBrokerHostSelector but we don't need the other condition.

I have added the fix.

@abhishekagarwal87 abhishekagarwal87 merged commit aaf0aaa into apache:master Aug 13, 2021
@abhishekagarwal87
Copy link
Contributor

Thank you, @kfaraz I have merged your PR.

@kfaraz kfaraz deleted the route_sql_queries branch August 23, 2021 06:47
@clintropolis clintropolis added this to the 0.22.0 milestone Sep 3, 2021
@clintropolis clintropolis mentioned this pull request Sep 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhishekagarwal87 abhishekagarwal87 abhishekagarwal87 approved these changes

@maytasm maytasm maytasm approved these changes

Assignees
No one assigned
Labels
Area - Router Area - SQL Design Review Release Notes
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kfaraz @abhishekagarwal87 @maytasm @clintropolis @asdf2014