You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sudheeshkatkam <gi...@git.apache.org> on 2016/08/10 23:01:36 UTC
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
GitHub user sudheeshkatkam opened a pull request:
https://github.com/apache/drill/pull/565
DRILL-4841: Use server event loop for web clients
@parthchandra please review (just the third commit).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sudheeshkatkam/drill DRILL-4841
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/565.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #565
----
commit 0889aa65023abf94c865589b7e4f5aae5baa9c6a
Author: Sudheesh Katkam <sk...@maprtech.com>
Date: 2016-03-09T18:34:26Z
DRILL-4606: HYGIENE
+ Merge DrillAutoCloseables and AuthCloseables
+ Remove unused imports
+ Expand * imports
commit 8cc6bc929b91a9f19f2ed8cbce293db8f86c1e48
Author: Sudheesh Katkam <sk...@maprtech.com>
Date: 2016-04-15T05:25:02Z
DRILL-4606: CORE
+ Add DrillClient.Builder helper class to create DrillClient objects
+ Deprecate 8 constructors and DrillClientFactory
+ Reorganize and document DrillClient
commit b5ee0fb6e2fd99c5e700a60261d8990642350983
Author: Sudheesh Katkam <sk...@maprtech.com>
Date: 2016-08-10T22:39:41Z
DRILL-4841: Use server event loop for web clients
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82267251
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given allocator.
+ *
+ * @param allocator buffer allocator
+ * @return this builder
+ */
+ public Builder setAllocator(final BufferAllocator allocator) {
+ this.allocator = allocator;
+ return this;
+ }
+
+ /**
+ * Sets the {@link ClusterCoordinator cluster coordinator} that this client
+ * registers with. If this is not set and the this client does not use a
+ * {@link #setDirectConnection direct connection}, a cluster coordinator will
+ * be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given coordinator.
+ *
+ * @param clusterCoordinator cluster coordinator
+ * @return this builder
+ */
+ public Builder setClusterCoordinator(final ClusterCoordinator clusterCoordinator) {
+ this.clusterCoordinator = clusterCoordinator;
+ return this;
+ }
+
+ /**
+ * Sets the event loop group that to be used by the client. If this is not set,
--- End diff --
Event loop for what? Why would I want to provide my own? What are the constraints on the event loop?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82295775
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -89,79 +88,175 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.util.concurrent.AbstractCheckedFuture;
+import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.SettableFuture;
/**
* Thin wrapper around a UserClient that handles connect/close and transforms
* String into ByteBuf.
+ *
+ * To create non-default objects, use {@link DrillClient.Builder the builder class}.
+ * E.g.
+ * <code>
+ * DrillClient client = DrillClient.newBuilder()
+ * .setConfig(...)
+ * .setIsDirectConnection(true)
+ * .build();
+ * </code>
+ *
+ * Except for {@link #runQuery} and {@link #cancelQuery}, this class is generally not thread safe.
*/
public class DrillClient implements Closeable, ConnectionThrottle {
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillClient.class);
private static final ObjectMapper objectMapper = new ObjectMapper();
private final DrillConfig config;
- private UserClient client;
- private UserProperties props = null;
- private volatile ClusterCoordinator clusterCoordinator;
- private volatile boolean connected = false;
private final BufferAllocator allocator;
- private int reconnectTimes;
- private int reconnectDelay;
- private boolean supportComplexTypes;
- private final boolean ownsZkConnection;
+ private final EventLoopGroup eventLoopGroup;
+ private final ExecutorService executor;
+ private final boolean isDirectConnection;
+ private final int reconnectTimes;
+ private final int reconnectDelay;
+
+ // TODO: clusterCoordinator should be initialized in the constructor.
+ // Currently, initialization is tightly coupled with #connect.
+ private ClusterCoordinator clusterCoordinator;
+
+ // checks if this client owns these resources (used when closing)
private final boolean ownsAllocator;
- private final boolean isDirectConnection; // true if the connection bypasses zookeeper and connects directly to a drillbit
- private EventLoopGroup eventLoopGroup;
- private ExecutorService executor;
+ private final boolean ownsZkConnection;
+ private final boolean ownsEventLoopGroup;
+ private final boolean ownsExecutor;
+
+ // once #setSupportComplexTypes() is removed, make this final
+ private boolean supportComplexTypes;
+
+ private UserClient client;
+ private UserProperties props;
+ private boolean connected;
- public DrillClient() throws OutOfMemoryException {
- this(DrillConfig.create(), false);
+ public DrillClient() {
+ this(newBuilder());
}
- public DrillClient(boolean isDirect) throws OutOfMemoryException {
- this(DrillConfig.create(), isDirect);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(boolean isDirect) {
+ this(newBuilder()
+ .setDirectConnection(isDirect));
}
- public DrillClient(String fileName) throws OutOfMemoryException {
- this(DrillConfig.create(fileName), false);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(String fileName) {
+ this(newBuilder()
+ .setConfigFromFile(fileName));
}
- public DrillClient(DrillConfig config) throws OutOfMemoryException {
- this(config, null, false);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config) {
+ this(newBuilder()
+ .setConfig(config));
}
- public DrillClient(DrillConfig config, boolean isDirect)
- throws OutOfMemoryException {
- this(config, null, isDirect);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, boolean isDirect) {
+ this(newBuilder()
+ .setConfig(config)
+ .setDirectConnection(isDirect));
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator)
- throws OutOfMemoryException {
- this(config, coordinator, null, false);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator) {
+ this(newBuilder()
+ .setConfig(config)
+ .setClusterCoordinator(coordinator));
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator, boolean isDirect)
- throws OutOfMemoryException {
- this(config, coordinator, null, isDirect);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator, boolean isDirect) {
+ this(newBuilder()
+ .setConfig(config)
+ .setClusterCoordinator(coordinator)
+ .setDirectConnection(isDirect));
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator)
- throws OutOfMemoryException {
- this(config, coordinator, allocator, false);
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator) {
+ this(newBuilder()
+ .setConfig(config)
+ .setClusterCoordinator(coordinator)
+ .setAllocator(allocator));
}
- public DrillClient(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator, boolean isDirect) {
- // if isDirect is true, the client will connect directly to the drillbit instead of
- // going thru the zookeeper
- this.isDirectConnection = isDirect;
- this.ownsZkConnection = coordinator == null && !isDirect;
- this.ownsAllocator = allocator == null;
- this.allocator = ownsAllocator ? RootAllocatorFactory.newRoot(config) : allocator;
- this.config = config;
- this.clusterCoordinator = coordinator;
+ /**
+ * @deprecated Create a DrillClient using {@link DrillClient.Builder}.
+ */
+ @Deprecated
+ public DrillClient(DrillConfig config, ClusterCoordinator coordinator, BufferAllocator allocator,
+ boolean isDirect) {
+ this(newBuilder()
+ .setConfig(config)
+ .setClusterCoordinator(coordinator)
+ .setAllocator(allocator)
+ .setDirectConnection(isDirect));
+ }
+
+ // used by DrillClient.Builder
+ private DrillClient(final Builder builder) {
+ this.config = builder.config != null ? builder.config : DrillConfig.create();
+
+ this.ownsAllocator = builder.allocator == null;
+ this.allocator = !ownsAllocator ? builder.allocator : RootAllocatorFactory.newRoot(config);
+
+ this.isDirectConnection = builder.isDirectConnection;
+ this.ownsZkConnection = builder.clusterCoordinator == null && !isDirectConnection;
+ this.clusterCoordinator = builder.clusterCoordinator; // could be null
+
+ this.ownsEventLoopGroup = builder.eventLoopGroup == null;
+ this.eventLoopGroup = !ownsEventLoopGroup ? builder.eventLoopGroup :
+ TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.CLIENT_RPC_THREADS), "Client-");
--- End diff --
Factor this out into a separate function? Creating a thread pool seems distinct enough that it should be separate from setting config options...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82102648
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java ---
@@ -53,6 +54,8 @@ public BootStrapContext(DrillConfig config, ScanResult classpathScan) {
this.loop2 = TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.BIT_SERVER_RPC_THREADS), "BitClient-");
// Note that metrics are stored in a static instance
this.metrics = DrillMetrics.getRegistry();
+ this.userLoopGroup = TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.USER_SERVER_RPC_THREADS),
--- End diff --
Please add a comment here stating that "userLoopGroup" threads will be used for "UserServerRPCThreads" and "ClientRPCThreads" in WebServer DrillClient instance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82267594
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given allocator.
+ *
+ * @param allocator buffer allocator
+ * @return this builder
+ */
+ public Builder setAllocator(final BufferAllocator allocator) {
+ this.allocator = allocator;
+ return this;
+ }
+
+ /**
+ * Sets the {@link ClusterCoordinator cluster coordinator} that this client
+ * registers with. If this is not set and the this client does not use a
+ * {@link #setDirectConnection direct connection}, a cluster coordinator will
+ * be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given coordinator.
+ *
+ * @param clusterCoordinator cluster coordinator
+ * @return this builder
+ */
+ public Builder setClusterCoordinator(final ClusterCoordinator clusterCoordinator) {
+ this.clusterCoordinator = clusterCoordinator;
+ return this;
+ }
+
+ /**
+ * Sets the event loop group that to be used by the client. If this is not set,
+ * an event loop group will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given event loop group.
+ *
+ * @param eventLoopGroup event loop group
+ * @return this builder
+ */
+ public Builder setEventLoopGroup(final EventLoopGroup eventLoopGroup) {
+ this.eventLoopGroup = eventLoopGroup;
+ return this;
+ }
+
+ /**
+ * Sets the executor service to be used by the client. If this is not set,
+ * an executor will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given executor.
+ *
+ * @param executor executor service
+ * @return this builder
+ */
+ public Builder setExecutorService(final ExecutorService executor) {
+ this.executor = executor;
+ return this;
+ }
+
+ /**
+ * Sets whether the application is willing to accept complex types (Map, Arrays)
--- End diff --
Default value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82265866
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
--- End diff --
Seems useful/visible enough to justify being a top-level class, with its own file. That is, any reason not to say:
DrillClient client = new ClientBuilder( ) ... .build( );
Rather than:
DrillClient client = DrillClient.builder( ) ... .build( );
Because the Builder becomes the gateway to Drill, having extensive JavaDoc comments would be very helpful. See suggestions below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82266459
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
--- End diff --
Better to set this based on a File (or, for the trendy, Path) object, since that is the Java standard for local files.
Does this replace the default class-path config? Or, is this added to the defaults?
Can I use this with the above? Or, can I set either the config directory OR via a file? If one or the other, should we check that case and throw an IllegalStateException (unchecked) or the like?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82267401
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given allocator.
+ *
+ * @param allocator buffer allocator
+ * @return this builder
+ */
+ public Builder setAllocator(final BufferAllocator allocator) {
+ this.allocator = allocator;
+ return this;
+ }
+
+ /**
+ * Sets the {@link ClusterCoordinator cluster coordinator} that this client
+ * registers with. If this is not set and the this client does not use a
+ * {@link #setDirectConnection direct connection}, a cluster coordinator will
+ * be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given coordinator.
+ *
+ * @param clusterCoordinator cluster coordinator
+ * @return this builder
+ */
+ public Builder setClusterCoordinator(final ClusterCoordinator clusterCoordinator) {
+ this.clusterCoordinator = clusterCoordinator;
+ return this;
+ }
+
+ /**
+ * Sets the event loop group that to be used by the client. If this is not set,
+ * an event loop group will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given event loop group.
+ *
+ * @param eventLoopGroup event loop group
+ * @return this builder
+ */
+ public Builder setEventLoopGroup(final EventLoopGroup eventLoopGroup) {
+ this.eventLoopGroup = eventLoopGroup;
+ return this;
+ }
+
+ /**
+ * Sets the executor service to be used by the client. If this is not set,
--- End diff --
What is an executor service and why would I want to create my own?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82269228
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillUserPrincipal.java ---
@@ -118,8 +118,11 @@ public AnonDrillUserPrincipal(final DrillbitContext drillbitContext) {
public DrillClient getDrillClient() throws IOException {
try {
// Create a DrillClient
- drillClient = new DrillClient(drillbitContext.getConfig(),
- drillbitContext.getClusterCoordinator(), drillbitContext.getAllocator());
+ drillClient = DrillClient.newBuilder()
--- End diff --
Is the context something we should expose as part of our changes:
DrillbitContext context = ...
DrillClient client = DrillClient.builder( )
.fromContext( context )
.build( );
Doing the above might simplify test code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82267107
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given allocator.
+ *
+ * @param allocator buffer allocator
+ * @return this builder
+ */
+ public Builder setAllocator(final BufferAllocator allocator) {
+ this.allocator = allocator;
+ return this;
+ }
+
+ /**
+ * Sets the {@link ClusterCoordinator cluster coordinator} that this client
--- End diff --
When would I use this rather than letting Drill create it from the config? Would I do this if I have multiple Drill clients in a single app? Or, will the underlying code know to share the same coordinator?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82269525
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestDistributedFragmentRun.java ---
@@ -41,7 +41,11 @@
public void oneBitOneExchangeOneEntryRun() throws Exception{
RemoteServiceSet serviceSet = RemoteServiceSet.getLocalServiceSet();
- try(Drillbit bit1 = new Drillbit(CONFIG, serviceSet); DrillClient client = new DrillClient(CONFIG, serviceSet.getCoordinator());){
+ try(Drillbit bit1 = new Drillbit(CONFIG, serviceSet);
--- End diff --
This pattern show up over and over. Should it just be moved into a function rather than as a duplicated "code injection" in a zillion files?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82268707
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/AbstractDrillLoginService.java ---
@@ -46,8 +46,11 @@ protected DrillClient createDrillClient(final String userName, final String pass
try {
// Create a DrillClient
- drillClient = new DrillClient(drillbitContext.getConfig(),
- drillbitContext.getClusterCoordinator(), drillbitContext.getAllocator());
+ drillClient = DrillClient.newBuilder()
+ .setConfig(drillbitContext.getConfig())
+ .setClusterCoordinator(drillbitContext.getClusterCoordinator())
+ .setAllocator(drillbitContext.getAllocator())
+ .build();
--- End diff --
Should the builder be extended to also (optionally) do connect? Or, return a connect builder?
Here I'm thinking that it would be helpful to have a single builder gather things like the user name property and the connection string (which would seem to be a DrillClient parameter but is actually a connection property.)
That is, either:
DrillClient.builder( ) ...
.withUser( userName )
.connectTo( "myHost", 1234 )
.build( );
Or
DrillClient.builder( ) ...
.buildClient( )
.withUser( userName )
.connectTo( "myHost", 1234 )
.connect( );
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82267967
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given allocator.
+ *
+ * @param allocator buffer allocator
+ * @return this builder
+ */
+ public Builder setAllocator(final BufferAllocator allocator) {
+ this.allocator = allocator;
+ return this;
+ }
+
+ /**
+ * Sets the {@link ClusterCoordinator cluster coordinator} that this client
+ * registers with. If this is not set and the this client does not use a
+ * {@link #setDirectConnection direct connection}, a cluster coordinator will
+ * be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given coordinator.
+ *
+ * @param clusterCoordinator cluster coordinator
+ * @return this builder
+ */
+ public Builder setClusterCoordinator(final ClusterCoordinator clusterCoordinator) {
+ this.clusterCoordinator = clusterCoordinator;
+ return this;
+ }
+
+ /**
+ * Sets the event loop group that to be used by the client. If this is not set,
+ * an event loop group will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given event loop group.
+ *
+ * @param eventLoopGroup event loop group
+ * @return this builder
+ */
+ public Builder setEventLoopGroup(final EventLoopGroup eventLoopGroup) {
+ this.eventLoopGroup = eventLoopGroup;
+ return this;
+ }
+
+ /**
+ * Sets the executor service to be used by the client. If this is not set,
+ * an executor will be created based on the configuration.
+ *
+ * If this is set, the caller is responsible for closing the given executor.
+ *
+ * @param executor executor service
+ * @return this builder
+ */
+ public Builder setExecutorService(final ExecutorService executor) {
+ this.executor = executor;
+ return this;
+ }
+
+ /**
+ * 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.
+ *
+ * @param supportComplexTypes if client accepts complex types
+ * @return this builder
+ */
+ public Builder setSupportsComplexTypes(final boolean supportComplexTypes) {
+ this.supportComplexTypes = supportComplexTypes;
+ return this;
+ }
+
+ /**
+ * Sets whether the client will connect directly to the drillbit instead of going
--- End diff --
Here (or in the class comment), explain how to use this with an embedded Drillbit. There is no ZK in that case. Would I use a direct connection for embedded?
If I choose to use embedded, where do I pass in the actual host:port information? Should this method take that info and, in so doing, implicitly set the "is direct" flag?
What is the default?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82266114
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
--- End diff --
Scenario? What is used by default? An empty config? One found on the class path?
What if I want to use the class-path (default) Drill config, but add my own customizations (as for tests)? Should we explain how to do this, or provide a method to help:
withExtraConfig( Config props )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82263890
--- Diff: common/src/main/java/org/apache/drill/common/AutoCloseables.java ---
@@ -87,4 +87,29 @@ public static void close(Iterable<? extends AutoCloseable> ac) throws Exception
throw topLevelException;
}
}
+
+ /**
+ * close() an {@see java.lang.AutoCloseable} without throwing a (checked)
+ * {@see java.lang.Exception}. This wraps the close() call with a
+ * try-catch that will rethrow an Exception wrapped with a
+ * {@see java.lang.RuntimeException}, providing a way to call close()
+ * without having to do the try-catch everywhere or propagate the Exception.
+ *
+ * @param autoCloseable the AutoCloseable to close; may be null
+ * @throws RuntimeException if an Exception occurs; the Exception is
+ * wrapped by the RuntimeException
+ */
+ public static void closeNoChecked(final AutoCloseable autoCloseable) {
--- End diff --
closeUnchecked ?
But, we are "checking", the "checked" has to do with the Exception type.
Maybe cleanClose( ) for this function. Then, add a "closeSilently" to catch and ignore close exceptions. (The closeSilently is handy in the case when, say, a file is full, a write failed, and the close will also fail because it still can't flush pending buffers.) There are other functions to the to silent close, but might be handy to have them in one place.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82266574
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
--- End diff --
DrillConfig objects are pretty complex. Should we accept the underlying Config (TypeSafe object) instead/in addition?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82264416
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -89,79 +88,175 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.util.concurrent.AbstractCheckedFuture;
+import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.SettableFuture;
/**
* Thin wrapper around a UserClient that handles connect/close and transforms
* String into ByteBuf.
+ *
+ * To create non-default objects, use {@link DrillClient.Builder the builder class}.
+ * E.g.
+ * <code>
+ * DrillClient client = DrillClient.newBuilder()
+ * .setConfig(...)
+ * .setIsDirectConnection(true)
--- End diff --
setDirectConnection
The "setIs" form is rather awkward. The typical convention is:
setSomething( boolean flag )
boolean isSomething( )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #565: DRILL-4841: Use server event loop for web clients
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/565#discussion_r82266904
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -688,4 +802,142 @@ public DrillBuf getBuffer() {
return null;
}
}
+
+ /**
+ * Return a new {@link DrillClient.Builder Drill client builder}.
+ * @return a new builder
+ */
+ public static Builder newBuilder() {
+ return new Builder();
+ }
+
+ /**
+ * Helper class to construct a {@link DrillClient Drill client}.
+ */
+ public static class Builder {
+
+ private DrillConfig config;
+ private BufferAllocator allocator;
+ private ClusterCoordinator clusterCoordinator;
+ private EventLoopGroup eventLoopGroup;
+ private ExecutorService executor;
+
+ // defaults
+ private boolean supportComplexTypes = true;
+ private boolean isDirectConnection = false;
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client.
+ *
+ * @param drillConfig drill configuration
+ * @return this builder
+ */
+ public Builder setConfig(DrillConfig drillConfig) {
+ this.config = drillConfig;
+ return this;
+ }
+
+ /**
+ * Sets the {@link DrillConfig configuration} for this client based on the given file.
+ *
+ * @param fileName configuration file name
+ * @return this builder
+ */
+ public Builder setConfigFromFile(final String fileName) {
+ this.config = DrillConfig.create(fileName);
+ return this;
+ }
+
+ /**
+ * Sets the {@link BufferAllocator buffer allocator} to be used by this client.
+ * If this is not set, an allocator will be created based on the configuration.
--- End diff --
But, what if I don't provide a config? Or, create my own? What do I have to set in that config to make it work?
More to the point, WHEN do I have to provide my own root? When I do multiple clients in a single app? If so, can we provide a short example showing how this works?
Even easier, can the builder itself create a static allocator that it shares across connections?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---