Add BrokerClient implementation
#17382
Conversation
…lan. - Add BrokerClient that leverages the ServiceClient functionality. Add a couple of client APIs that are useful to the scheduled batch supervisor implementation. - Add ExplainPlanInformation class that contains information about a single plan. The BrokerClient leverages this.
| * classes present in the sql module. | ||
| * </p> | ||
| */ | ||
| public class BrokerServiceModule extends ServiceClientModule |
There was a problem hiding this comment.
Nit:
Why extend ServiceClientModule?
I think the only required method from ServiceClientModule is makeServiceClientFactory().
I guess it is okay to copy over that method.
There was a problem hiding this comment.
yeah, makeServiceClientFactory() and the constants for number of retries and connection threads to use. So I left it to avoid duplication.
| * at least for the native query explain, but there's currently no use case for it. | ||
| * </p> | ||
| */ | ||
| public class ExplainPlanInformation |
| return attributes; | ||
| } | ||
|
|
||
| private static class ExplainAttributesDeserializer extends JsonDeserializer<ExplainAttributes> |
There was a problem hiding this comment.
Why is the custom deserializer needed? I don't see it doing anything special.
There was a problem hiding this comment.
The explain plan response contains objects encoded as strings, so jackson needs a way to convert the Json string into individual attributes. Another option is to add a @JsonCreator String constructor in ExplainAttributes itself besides the existing constructor, which would essentially do the same thing as ExplainAttributesDeserializer#deserialize.
Without this, the tests in ExplainPlanTest would fail with an error:
by: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `org.apache.druid.sql.calcite.planner.ExplainAttributes` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('{"statementType":"INSERT","targetDataSource":"foo","pa...
For now, I have added a comment for the deserializer here since this is needed only for ExplainPlan. Please let me know if you prefer one approach or the other.
sql/src/main/java/org/apache/druid/sql/calcite/planner/ExplainAttributes.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @param sqlQuery the SQL query for which the {@code EXPLAIN PLAN FOR} information is to be fetched | ||
| */ | ||
| ListenableFuture<List<ExplainPlanInformation>> fetchExplainPlanInformation(SqlQuery sqlQuery); |
There was a problem hiding this comment.
| ListenableFuture<List<ExplainPlanInformation>> fetchExplainPlanInformation(SqlQuery sqlQuery); | |
| ListenableFuture<List<ExplainPlanInformation>> fetchExplainPlan(SqlQuery sqlQuery); |
| ListenableFuture<SqlTaskStatus> submitSqlTask(SqlQuery sqlQuery); | ||
|
|
||
| /** | ||
| * Fetches the explain plan information for the given {@code sqlQuery}. |
There was a problem hiding this comment.
It would be nice if the javadoc also mentioned the HTTP endpoint being hit.
This patch is extracted from 17353 to make reviewing easier. It includes additional test coverage and javadocs compared to the original patch.
Changes:
BrokerClientandBrokerClientImplto the sql package that leverages theServiceClientfunctionality; similar toOverlordClientandCoordinatorClientimplementations in the server module.submitSqlTask()andfetchExplainPlanInformation().org.apache.druid.discovery.BrokerClientin the server module that's used by MSQE for a specific use case. This client has a weird contract though, and its retry mechanism is not robust. We should plan to remove it in the future, but for now, it has been marked as deprecated in favor of the newBrokerClientadded in this patch.ExplainPlanInformationthat encapsulates explain plan info.ExplainAttributesTesta bit and added serde verification.In this patch, the
BrokerClientand the relatedBrokerServiceModulethat injects it is unused. It will be used by the scheduled batch supervisor running on the Overlord to interact with the Broker.This PR has: