-
Notifications
You must be signed in to change notification settings - Fork 985
DRILL-4369: Exchange name and version infos during handshake #622
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /******************************************************************************* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| ******************************************************************************/ | ||
|
|
||
| <@pp.dropOutputFile /> | ||
|
|
||
| <@pp.changeOutputFile name="/org/apache/drill/common/util/DrillVersionInfo.java" /> | ||
|
|
||
| <#include "/@includes/license.ftl" /> | ||
|
|
||
| package org.apache.drill.common.util; | ||
|
|
||
| import java.text.MessageFormat; | ||
| import java.text.ParseException; | ||
|
|
||
| /* | ||
| * This file is generated with Freemarker using the template src/main/codegen/templates/DrillVersionInfo.java | ||
| */ | ||
| /** | ||
| * Give access to Drill version as captured during the build | ||
| * | ||
| * <strong>Caution</strong> don't rely on major, minor and patch versions only to compare two | ||
| * Drill versions. Instead you should use the whole string, and apply the same semver algorithm | ||
| * as Maven (see {@code org.apache.maven.artifact.versioning.ComparableVersion}). | ||
| * | ||
| */ | ||
| public class DrillVersionInfo { | ||
| private static final String VERSION = "${maven.project.version}"; | ||
|
|
||
| private static final int MAJOR_VERSION = ${maven.project.artifact.selectedVersion.majorVersion}; | ||
| private static final int MINOR_VERSION = ${maven.project.artifact.selectedVersion.minorVersion}; | ||
| private static final int PATCH_VERSION = ${maven.project.artifact.selectedVersion.incrementalVersion}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allow a suffix? Drill 1.9.0-SNAPSHOT or Drill 1.9.0-Fix17 There is a big difference between, say a SNAPSHOT version and the released version...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, that's why I added a big javadoc notice to rely on the version string for semver. Technically we should also add the build version if we wanted to be maven compatible |
||
|
|
||
| /** | ||
| * Get the Drill version from pom | ||
| * @return the version number as x.y.z | ||
| */ | ||
| public static String getVersion() { | ||
| return VERSION; | ||
| } | ||
|
|
||
| /** | ||
| * Get the Drill major version from pom | ||
| * @return x if assuming the version number is x.y.z | ||
| */ | ||
| public static int getMajorVersion() { | ||
| return MAJOR_VERSION; | ||
| } | ||
|
|
||
| /** | ||
| * Get the Drill minor version from pom | ||
| * @return y if assuming the version number is x.y.z | ||
| */ | ||
| public static int getMinorVersion() { | ||
| return MINOR_VERSION; | ||
| } | ||
|
|
||
| /** | ||
| * Get the Drill patch version from pom | ||
| * @return z if assuming the version number is x.y.z(-suffix) | ||
| */ | ||
| public static int getPatchVersion() { | ||
| return PATCH_VERSION; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| package org.apache.drill.exec.client; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkArgument; | ||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
| import static com.google.common.base.Preconditions.checkState; | ||
| import static org.apache.drill.exec.proto.UserProtos.QueryResultsMode.STREAM_FULL; | ||
| import static org.apache.drill.exec.proto.UserProtos.RunQuery.newBuilder; | ||
|
|
@@ -66,6 +67,7 @@ | |
| import org.apache.drill.exec.proto.UserProtos.PreparedStatementHandle; | ||
| import org.apache.drill.exec.proto.UserProtos.Property; | ||
| import org.apache.drill.exec.proto.UserProtos.QueryPlanFragments; | ||
| import org.apache.drill.exec.proto.UserProtos.RpcEndpointInfos; | ||
| import org.apache.drill.exec.proto.UserProtos.RpcType; | ||
| import org.apache.drill.exec.proto.UserProtos.RunQuery; | ||
| import org.apache.drill.exec.proto.UserProtos.UserProperties; | ||
|
|
@@ -98,6 +100,8 @@ | |
| * String into ByteBuf. | ||
| */ | ||
| public class DrillClient implements Closeable, ConnectionThrottle { | ||
| public static final String DEFAULT_CLIENT_NAME = "Apache Drill Java client"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't there a C++ client? Perhaps the default name should be "Undefined Drill client" to allow for this case.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The C++ client will have a default Apache Drill C++ client, with an option to override it by the user
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, since I already have a big C++ patch in progress, the C++ change will be done in that branch |
||
|
|
||
| private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillClient.class); | ||
|
|
||
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
|
|
@@ -115,6 +119,7 @@ public class DrillClient implements Closeable, ConnectionThrottle { | |
| private final boolean isDirectConnection; // true if the connection bypasses zookeeper and connects directly to a drillbit | ||
| private EventLoopGroup eventLoopGroup; | ||
| private ExecutorService executor; | ||
| private String clientName = DEFAULT_CLIENT_NAME; | ||
|
|
||
| public DrillClient() throws OutOfMemoryException { | ||
| this(DrillConfig.create(), false); | ||
|
|
@@ -175,6 +180,23 @@ public void setAutoRead(boolean enableAutoRead) { | |
| client.setAutoRead(enableAutoRead); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the client name. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ambiguous. What "client name"? The name of the user? The name of the client application? The name of the client API? ... Please explain.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Client name is more of the user agent, like JDBC client, ODBC client, C++ connector, etc... |
||
| * | ||
| * If not set, default is {@code DrillClient#DEFAULT_CLIENT_NAME}. | ||
| * | ||
| * @param name the client name | ||
| * | ||
| * @throws IllegalStateException if called after a connection has been established. | ||
| * @throws NullPointerException if client name is null | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old clients won't know to provide a name. The name will thus be null. But, we throw an NPE in this case. Two issues. First, is an NPE the right way to communicate to a (new) client that it failed to follow the (now required) protocol? Should this be translated to some kind of protocol exception somewhere? If so, how will we know that this is an NPE from a protocol error vs just a code bug? Second, where do we handle the case that a client does not provide a name? There should be a reasonable default (which seems to be the default name, but this bit is no exactly clear.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DrillClient have a default name, so name won't be null. Setting a null name though will cause an exception... |
||
| */ | ||
| public void setClientName(String name) { | ||
| if (connected) { | ||
| throw new IllegalStateException("Attempted to modify client connection property after connection has been established."); | ||
| } | ||
| this.clientName = checkNotNull(name, "client name should not be null"); | ||
| } | ||
|
|
||
| /** | ||
| * Sets whether the application is willing to accept complex types (Map, Arrays) in the returned result set. | ||
| * Default is {@code true}. If set to {@code false}, the complex types are returned as JSON encoded VARCHAR type. | ||
|
|
@@ -252,7 +274,7 @@ protected void afterExecute(final Runnable r, final Throwable t) { | |
| super.afterExecute(r, t); | ||
| } | ||
| }; | ||
| client = new UserClient(config, supportComplexTypes, allocator, eventLoopGroup, executor); | ||
| client = new UserClient(clientName, config, supportComplexTypes, allocator, eventLoopGroup, executor); | ||
| logger.debug("Connecting to server {}:{}", endpoint.getAddress(), endpoint.getUserPort()); | ||
| connect(endpoint); | ||
| connected = true; | ||
|
|
@@ -330,6 +352,19 @@ public void close() { | |
| connected = false; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Return the server infos. Only available after connecting | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please explain what the "server info" is and we get it from the client object. Is this information about the server associated with this particular client connection? Information provided to the server by this client? Something else? |
||
| * | ||
| * The result might be null if the server doesn't provide the informations. | ||
| * | ||
| * @return the server informations, or null if not connected or if the server | ||
| * doesn't provide the information | ||
| */ | ||
| public RpcEndpointInfos getServerInfos() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Infos -> Info "Information" is already plural.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I learned something :) |
||
| return client != null ? client.getServerInfos() : null; | ||
| } | ||
|
|
||
| /** | ||
| * Submits a string based query plan for execution and returns the result batches. Supported query types are: | ||
| * <p><ul> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,6 @@ | |
| */ | ||
| package org.apache.drill.exec.rpc.user; | ||
|
|
||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.channel.EventLoopGroup; | ||
|
|
||
| import java.util.concurrent.Executor; | ||
|
|
||
| import org.apache.drill.common.config.DrillConfig; | ||
|
|
@@ -39,6 +36,7 @@ | |
| import org.apache.drill.exec.proto.UserProtos.GetTablesResp; | ||
| import org.apache.drill.exec.proto.UserProtos.HandshakeStatus; | ||
| import org.apache.drill.exec.proto.UserProtos.QueryPlanFragments; | ||
| import org.apache.drill.exec.proto.UserProtos.RpcEndpointInfos; | ||
| import org.apache.drill.exec.proto.UserProtos.RpcType; | ||
| import org.apache.drill.exec.proto.UserProtos.RunQuery; | ||
| import org.apache.drill.exec.proto.UserProtos.UserProperties; | ||
|
|
@@ -55,14 +53,20 @@ | |
|
|
||
| import com.google.protobuf.MessageLite; | ||
|
|
||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.channel.EventLoopGroup; | ||
|
|
||
| public class UserClient extends BasicClientWithConnection<RpcType, UserToBitHandshake, BitToUserHandshake> { | ||
| private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(UserClient.class); | ||
|
|
||
| private final QueryResultHandler queryResultHandler = new QueryResultHandler(); | ||
| private final String clientName; | ||
| private RpcEndpointInfos serverInfos = null; | ||
|
|
||
| private boolean supportComplexTypes = true; | ||
|
|
||
| public UserClient(DrillConfig config, boolean supportComplexTypes, BufferAllocator alloc, | ||
| EventLoopGroup eventLoopGroup, Executor eventExecutor) { | ||
| public UserClient(String clientName, DrillConfig config, boolean supportComplexTypes, | ||
| BufferAllocator alloc, EventLoopGroup eventLoopGroup, Executor eventExecutor) { | ||
| super( | ||
| UserRpcConfig.getMapping(config, eventExecutor), | ||
| alloc, | ||
|
|
@@ -71,9 +75,14 @@ public UserClient(DrillConfig config, boolean supportComplexTypes, BufferAllocat | |
| BitToUserHandshake.class, | ||
| BitToUserHandshake.PARSER, | ||
| "user client"); | ||
| this.clientName = clientName; | ||
| this.supportComplexTypes = supportComplexTypes; | ||
| } | ||
|
|
||
| public RpcEndpointInfos getServerInfos() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Infos -> Info |
||
| return serverInfos; | ||
| } | ||
|
|
||
| public void submitQuery(UserResultsListener resultsListener, RunQuery query) { | ||
| send(queryResultHandler.getWrappedListener(resultsListener), RpcType.RUN_QUERY, query, QueryId.class); | ||
| } | ||
|
|
@@ -85,7 +94,8 @@ public void connect(RpcConnectionHandler<ServerConnection> handler, DrillbitEndp | |
| .setSupportListening(true) | ||
| .setSupportComplexTypes(supportComplexTypes) | ||
| .setSupportTimeout(true) | ||
| .setCredentials(credentials); | ||
| .setCredentials(credentials) | ||
| .setClientInfos(UserRpcUtils.getRpcEndpointInfos(clientName)); | ||
|
|
||
| if (props != null) { | ||
| hsBuilder.setProperties(props); | ||
|
|
@@ -141,6 +151,9 @@ protected Response handleReponse(ConnectionThrottle throttle, int rpcType, ByteB | |
| @Override | ||
| protected void validateHandshake(BitToUserHandshake inbound) throws RpcException { | ||
| // logger.debug("Handling handshake from bit to user. {}", inbound); | ||
| if (inbound.hasServerInfos()) { | ||
| serverInfos = inbound.getServerInfos(); | ||
| } | ||
| if (inbound.getStatus() != HandshakeStatus.SUCCESS) { | ||
| final String errMsg = String.format("Status: %s, Error Id: %s, Error message: %s", | ||
| inbound.getStatus(), inbound.getErrorId(), inbound.getErrorMessage()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.drill.exec.rpc.user; | ||
|
|
||
| import java.lang.management.ManagementFactory; | ||
| import java.lang.management.RuntimeMXBean; | ||
|
|
||
| import org.apache.drill.common.util.DrillVersionInfo; | ||
| import org.apache.drill.exec.proto.UserProtos.RpcEndpointInfos; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
|
|
||
| /** | ||
| * Utility class for User RPC | ||
| * | ||
| */ | ||
| public final class UserRpcUtils { | ||
| private UserRpcUtils() {} | ||
|
|
||
| /** | ||
| * Returns a {@code RpcEndpointInfos} instance | ||
| * | ||
| * The instance is populated based on Drill version informations | ||
| * from the classpath and runtime information for the application | ||
| * name. | ||
| * | ||
| * @param name the endpoint name. | ||
| * @return a {@code RpcEndpointInfos} instance | ||
| * @throws NullPointerException if name is null | ||
| */ | ||
| public static RpcEndpointInfos getRpcEndpointInfos(String name) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Infos -> Info |
||
| RuntimeMXBean mxBean = ManagementFactory.getRuntimeMXBean(); | ||
| RpcEndpointInfos infos = RpcEndpointInfos.newBuilder() | ||
| .setName(Preconditions.checkNotNull(name)) | ||
| .setApplication(mxBean.getName()) | ||
| .setVersion(DrillVersionInfo.getVersion()) | ||
| .setMajorVersion(DrillVersionInfo.getMajorVersion()) | ||
| .setMinorVersion(DrillVersionInfo.getMinorVersion()) | ||
| .setPatchVersion(DrillVersionInfo.getPatchVersion()) | ||
| .build(); | ||
|
|
||
| return infos; | ||
| } | ||
| } | ||
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.
Nice use of Freemarker!
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.
Credits to @julienledem for the idea
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.
:)
and @jaltekruse