DRILL-4841: Use server event loop for web clients by sudheeshkatkam · Pull Request #565 · apache/drill

sudheeshkatkam · 2016-08-10T23:01:33Z

@parthchandra please review (just the third commit).

+ Merge DrillAutoCloseables and AuthCloseables + Remove unused imports + Expand * imports

+ Add DrillClient.Builder helper class to create DrillClient objects + Deprecate 8 constructors and DrillClientFactory + Reorganize and document DrillClient

sohami · 2016-10-06T01:16:18Z

    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),


Please add a comment here stating that "userLoopGroup" threads will be used for "UserServerRPCThreads" and "ClientRPCThreads" in WebServer DrillClient instance.

paul-rogers

Mostly style/organization comments.

paul-rogers · 2016-10-06T19:20:49Z

+   * @throws RuntimeException if an Exception occurs; the Exception is
+   *   wrapped by the RuntimeException
+   */
+  public static void closeNoChecked(final AutoCloseable autoCloseable) {


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.

paul-rogers · 2016-10-06T19:23:40Z

+ * <code>
+ *   DrillClient client = DrillClient.newBuilder()
+ *       .setConfig(...)
+ *       .setIsDirectConnection(true)


setDirectConnection

The "setIs" form is rather awkward. The typical convention is:

setSomething( boolean flag )
boolean isSomething( )

paul-rogers · 2016-10-06T19:32:08Z

+  /**
+   * Helper class to construct a {@link DrillClient Drill client}.
+   */
+  public static class Builder {


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.

paul-rogers · 2016-10-06T19:33:43Z

+    private boolean isDirectConnection = false;
+
+    /**
+     * Sets the {@link DrillConfig configuration} for this client.


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 )

paul-rogers · 2016-10-06T19:35:48Z

+    }
+
+    /**
+     * Sets the {@link DrillConfig configuration} for this client based on the given file.


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?

paul-rogers · 2016-10-06T19:42:57Z

+    }
+
+    /**
+     * Sets whether the client will connect directly to the drillbit instead of going


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?

paul-rogers · 2016-10-06T19:46:41Z

+          .setConfig(drillbitContext.getConfig())
+          .setClusterCoordinator(drillbitContext.getClusterCoordinator())
+          .setAllocator(drillbitContext.getAllocator())
+          .build();


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( );

paul-rogers · 2016-10-06T19:49:25Z

        // Create a DrillClient
-        drillClient = new DrillClient(drillbitContext.getConfig(),
-            drillbitContext.getClusterCoordinator(), drillbitContext.getAllocator());
+        drillClient = DrillClient.newBuilder()


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.

paul-rogers · 2016-10-06T19:50:39Z

    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);


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?

paul-rogers · 2016-10-06T22:04:50Z

+
+    this.ownsEventLoopGroup = builder.eventLoopGroup == null;
+    this.eventLoopGroup = !ownsEventLoopGroup ? builder.eventLoopGroup :
+        TransportCheck.createEventLoopGroup(config.getInt(ExecConstants.CLIENT_RPC_THREADS), "Client-");


Factor this out into a separate function? Creating a thread pool seems distinct enough that it should be separate from setting config options...

vvysotskyi · 2019-07-17T09:19:04Z

@sudheeshkatkam, did you have a chance to address Pauls CR comments?

Sudheesh Katkam added 3 commits August 10, 2016 15:14

DRILL-4606: HYGIENE

0889aa6

+ Merge DrillAutoCloseables and AuthCloseables + Remove unused imports + Expand * imports

DRILL-4606: CORE

8cc6bc9

+ Add DrillClient.Builder helper class to create DrillClient objects + Deprecate 8 constructors and DrillClientFactory + Reorganize and document DrillClient

DRILL-4841: Use server event loop for web clients

b5ee0fb

sohami reviewed Oct 6, 2016

View reviewed changes

paul-rogers approved these changes Oct 6, 2016

View reviewed changes

vvysotskyi mentioned this pull request Jul 17, 2019

DRILL-4606: Add DrillClient.Builder class #480

Closed

Conversation

sudheeshkatkam commented Aug 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-rogers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-rogers Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvysotskyi commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paul-rogers Oct 6, 2016 •

edited

Loading