DRILL-5485: Remove WebServer dependency on DrillClient by sohami · Pull Request #829 · apache/drill

sohami · 2017-05-10T01:12:43Z

No description provided.

sudheeshkatkam · 2017-05-23T01:20:10Z

exec/java-exec/src/main/java/org/apache/drill/exec/rpc/AbstractUserClientConnectionWrapper.java

+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+public abstract class AbstractUserClientConnectionWrapper implements UserClientConnection {


Add some doc about how instances of sub-classes can only be used once.

And this classes itself doesn't wrap.. How about AbstractDisposableUserClientConnection?

sudheeshkatkam · 2017-05-23T01:20:51Z

exec/java-exec/src/main/java/org/apache/drill/exec/rpc/UserClientConnection.java

+   * @param listener
+   * @param result
+   */
+  void sendResult(RpcOutcomeListener<GeneralRPCProtos.Ack> listener, UserBitShared.QueryResult result);


Add imports for inner classes, here and everywhere else.

sudheeshkatkam · 2017-05-23T01:22:26Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java

+                .setUserName(sessionUserPrincipal.getName())
+                .build())
+            .withOptionManager(drillbitContext.getOptionManager())
+            .setSupportComplexTypes(drillbitContext.getConfig().getBoolean(ExecConstants.CLIENT_SUPPORT_COMPLEX_TYPES))


This is used to check if client can handle complex types, which is not based on config. Can the web client handle complex types?

Similar call below.

For DrillClient by default this is set as true based on the config only. I looked into the usage and based on this property in session, if false planner will introduce a project node to convert complex types to JSON string. On WebServer when data batch is received we convert each data type to it's string format (even the complex types). Hence it does support complex types since WebClient now will see all the data in Json string format and should be fine.

sudheeshkatkam · 2017-05-23T01:27:00Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java

-  public UserBitShared.QueryType getType() {
-    UserBitShared.QueryType type = UserBitShared.QueryType.SQL;
+  public QueryType getType() {
+    QueryType type = QueryType.SQL;


return QueryType.valueOf(queryType);

sudheeshkatkam · 2017-05-23T01:27:40Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java

-    }
+  public QueryResult run(final WorkManager workManager, final WebUserConnection webUserConnection) throws Exception {
+
+    final UserProtos.RunQuery runQuery = UserProtos.RunQuery.getDefaultInstance().newBuilderForType()


RunQuery.newBuilder().setType(...)...

sudheeshkatkam · 2017-05-23T16:08:13Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java

+        try {
+          // This can be slow as the underlying library will try to resolve the address
+          remoteAddress = new InetSocketAddress(InetAddress.getByName(request.getRemoteAddr()), request.getRemotePort());
+          session.setAttribute(SocketAddress.class.getSimpleName(), remoteAddress);


Attributes names too generic. Maybe use: "drill-user-session", "drill-socket-address", ...?

sudheeshkatkam · 2017-05-23T16:16:25Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserConnection.java

+
+  /**
+   * For anonymous WebUserSession it is called after each query request whereas for Authenticated WebUserSession
+   * it is never called.


Comment does not seem right. Looks to me that cleanupSession() is called after every query request, and the session resources are closed iff the user is anonymous.

sudheeshkatkam · 2017-05-23T17:33:07Z

exec/java-exec/src/main/java/org/apache/drill/exec/work/prepare/PreparedStatementProvider.java

-import io.netty.buffer.ByteBuf;
-import io.netty.channel.ChannelFuture;
+import static org.apache.drill.exec.ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS;
+import static org.apache.drill.exec.proto.UserProtos.RequestStatus.*;


expand imports

sudheeshkatkam · 2017-05-23T17:37:33Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java

    return new SessionHandler(sessionManager);
  }

+  private void clearSessionCustomAttributes(HttpSession session) {


How about encapsulating all the resources in one class (DrillWebSession)?

Only that instance would be an attribute on a http session.

This logic could be captured in DrillWebSession#close method.

WebUserConnection is a class which is encapsulating all the resources. I had the logic of close and cleanup in it earlier. But I Didn't set that as attribute of Http session object since in that case it will be reset for each request. Since there are not many resources which we are setting inside HttpSession and each of them are really a separate property for the session in itself, I guess keeping them separated should be fine.

(I somehow managed to delete this in my comment..) The life cycle of those resources could be placed together in one class, as resources are being initialized in one place but closed in different places.

Moved all the resources inside another class WebSessionResources which now manages their lifecycle.

sudheeshkatkam · 2017-05-23T20:29:04Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserConnection.java

+public class WebUserConnection extends AbstractUserClientConnectionWrapper implements ConnectionThrottle {
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(WebUserConnection.class);
+
+  /**


class level doc?

…roviders for Authenticated and Anonymous web users. NOTE: Updated to store the UserSession, BufferAllocator and other session states inside the HttpSession of Jetty instead of storing in DrillUserPrincipal. For each request now a new instance of WebUserConnection will be created. However for authenticated users the UserSession and other states will be re-used whereas for Anonymous Users it will created for each request and later re-cycled after query execution.

jinfengni · 2017-06-01T22:07:43Z

Seems @sohami has addressed @sudheeshkatkam 's comments. I'm going to merge this PR.

+1

1. Added WebUserConnection/AnonWebUserConnection and their providers for Authenticated and Anonymous web users. 2. Updated to store the UserSession, BufferAllocator and other session states inside the HttpSession of Jetty instead of storing in DrillUserPrincipal. For each request now a new instance of WebUserConnection will be created. However for authenticated users the UserSession and other states will be re-used whereas for Anonymous Users it will created for each request and later re-cycled after query execution. close apache#829

sohami force-pushed the DRILL-5485 branch from d5cdfa0 to 7132182 Compare May 10, 2017 04:45

sohami force-pushed the DRILL-5485 branch 2 times, most recently from d90af9e to 503f861 Compare May 20, 2017 04:03

sudheeshkatkam reviewed May 23, 2017

View reviewed changes

Sorabh Hamirwasia added 3 commits May 24, 2017 00:45

DRILL-5485: Remove WebServer dependency on DrillClient

9757430

DRILL-5485: Changes for code review feedback

5db9759

sohami force-pushed the DRILL-5485 branch from 503f861 to 5db9759 Compare May 24, 2017 07:59

DRILL-5485: Code review changes

0897385

asfgit closed this in 874bf62 Jun 3, 2017

Conversation

sohami commented May 10, 2017

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!

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!

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!

jinfengni commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants