[jira] [Commented] (DRILL-4606) Create DrillClient.Builder class

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DRILL-4606?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15317619#comment-15317619 ] 

ASF GitHub Bot commented on DRILL-4606:
---------------------------------------

Github user parthchandra commented on a diff in the pull request:

    https://github.com/apache/drill/pull/480#discussion_r65997345
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
    @@ -74,81 +78,173 @@
     import com.fasterxml.jackson.core.JsonProcessingException;
     import com.fasterxml.jackson.databind.ObjectMapper;
     import com.fasterxml.jackson.databind.node.ArrayNode;
    -import com.google.common.base.Strings;
    -import com.google.common.util.concurrent.AbstractCheckedFuture;
    -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;
     
    -  public DrillClient() throws OutOfMemoryException {
    -    this(DrillConfig.create(), false);
    +  // once #setSupportComplexTypes() is removed, make this final
    +  private boolean supportComplexTypes;
    +
    +  private UserClient client;
    +  private UserProperties props;
    +  private boolean connected;
    +
    +  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));
    +  }
    +
    +  /**
    +   * @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));
       }
     
    -  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;
    +  // 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-");
    +
    +    this.ownsExecutor = builder.executor == null;
    +    this.executor = !ownsExecutor ? builder.executor :
    +        new ThreadPoolExecutor(0, Integer.MAX_VALUE, 60L, TimeUnit.SECONDS,
    +            new SynchronousQueue<Runnable>(),
    +            new NamedThreadFactory("drill-client-executor-")) {
    +          @Override
    +          protected void afterExecute(final Runnable r, final Throwable t) {
    +            if (t != null) {
    +              logger.error(String.format("%s.run() leaked an exception.", r.getClass().getName()), t);
    +            }
    +            super.afterExecute(r, t);
    +          }
    +        };
    +
    +    this.supportComplexTypes = builder.supportComplexTypes;
    --- End diff --
    
    This is probably OK. This is the Java client and the client does, in fact, support complex types. 


> Create DrillClient.Builder class
> --------------------------------
>
>                 Key: DRILL-4606
>                 URL: https://issues.apache.org/jira/browse/DRILL-4606
>             Project: Apache Drill
>          Issue Type: Improvement
>            Reporter: Sudheesh Katkam
>            Assignee: Sudheesh Katkam
>
> + Create a helper class to build DrillClient instances, and deprecate DrillClient constructors
> + Allow DrillClient to specify an event loop group (so user event loop can be used for queries from Web API calls)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)