[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/993


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r146112692
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserConnection.java
 ---
@@ -124,6 +124,13 @@ public void sendData(RpcOutcomeListener listener, 
QueryWritableBatch result
 }
   }
 
+  /**
+   * Returns a DefaultChannelPromise which doesn't have reference to any 
actual channel but has an EventExecutor
+   * associated with it. In this case we use EventExecutor out of 
BitServer EventLoopGroup. Since there is no actual
+   * connection established using this class, hence the close event will 
never be fired by underlying layer and close
+   * future is set only when the WebSessionResources are closed.
--- End diff --

Based on previous conversation the assumption was unsecure path also 
maintains cookie or session. On looking more this proved to be false and we 
don't maintain any session for unsecure case. This means that the requests 
submitted without authentication are stateless and once the connection is 
broken by closing the browser session then the query will still continue 
running until explicitly cancelled from profile page. I didn't found any way in 
Jetty to get notification about connection close event. And looks like that 
won't be much helpful right now since we send the response back only after the 
query is completed and we have entire result. I have opened another JIRA 
[DRILL-5897](https://issues.apache.org/jira/browse/DRILL-5897) to see if we can 
improve the way we send result back to client or make unsecure case to also use 
session concept such that we can cancel the in-flight queries based on when 
session is invalidated.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145565896
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -204,9 +217,15 @@ public WebUserConnection provide() {
 
config.getLong(ExecConstants.HTTP_SESSION_MEMORY_RESERVATION),
 config.getLong(ExecConstants.HTTP_SESSION_MEMORY_MAXIMUM));
 
+// Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+// listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+// listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
+// all the running queries for this Web Client.
--- End diff --

The query is killed when the HttpSession is invalidated either by logout or 
by timeout in case of authenticated user session. But for unauthenticated or 
anonymous user we don't kill the query after HttpSession timeout since we don't 
maintain any session related information. The query will keep running unless it 
is explicitly cancelled from /profile page.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145565968
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -260,8 +282,14 @@ public WebUserConnection provide() {
 logger.trace("Failed to get the remote address of the http session 
request", ex);
   }
 
-  final WebSessionResources webSessionResources = new 
WebSessionResources(sessionAllocator,
-  remoteAddress, drillUserSession);
+  // Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+  // listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+  // listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
--- End diff --

Will fix the comment


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145568772
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserConnection.java
 ---
@@ -124,6 +124,13 @@ public void sendData(RpcOutcomeListener listener, 
QueryWritableBatch result
 }
   }
 
+  /**
+   * Returns a DefaultChannelPromise which doesn't have reference to any 
actual channel but has an EventExecutor
+   * associated with it. In this case we use EventExecutor out of 
BitServer EventLoopGroup. Since there is no actual
+   * connection established using this class, hence the close event will 
never be fired by underlying layer and close
+   * future is set only when the WebSessionResources are closed.
--- End diff --

The intent of putting this comment here was just as an FYI how the 
underlying CloseFuture is created which is returned by this function and 
stressting the fact that it doesn't have any actual physical connection 
associated with it.

WebUserConnection acts like an adapter between HttpSession and connection 
which Foreman expects with each submitted request. For authenticated user this 
adapter is used to get UserSession object which stores all the updated Drill 
session options changed over the time. 

For anonymous user this adapter is created fresh with each request. But as 
discussed in person I will make a change so that we can maintain session for 
anonymous user and have similar behavior like authenticated user. And when the 
HttpSession for unauthenticated user time's out then it will do clean up of all 
the web resources related to that session.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145565944
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -204,9 +217,15 @@ public WebUserConnection provide() {
 
config.getLong(ExecConstants.HTTP_SESSION_MEMORY_RESERVATION),
 config.getLong(ExecConstants.HTTP_SESSION_MEMORY_MAXIMUM));
 
+// Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+// listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+// listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
+// all the running queries for this Web Client.
+final ChannelPromise closeFuture = new DefaultChannelPromise(null, 
executor);
+
 // Create a WebSessionResource instance which owns the lifecycle 
of all the session resources.
-// Set this instance as an attribute of HttpSession, since it will 
be used until session is destroyed.
-webSessionResources = new WebSessionResources(sessionAllocator, 
remoteAddress, drillUserSession);
+// Set this instance as an attribute of HttpSession, since it will 
be used until session is destroyed
+webSessionResources = new WebSessionResources(sessionAllocator, 
remoteAddress, drillUserSession, closeFuture);
--- End diff --

Will fix the comment. Sorry for confusion.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145571583
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/WebSessionResourcesTest.java
 ---
@@ -0,0 +1,146 @@
+/*
+ * 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.server.rest;
+
+import io.netty.channel.ChannelPromise;
+import io.netty.channel.DefaultChannelPromise;
+import io.netty.util.concurrent.EventExecutor;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.drill.exec.rpc.TransportCheck;
+import org.apache.drill.exec.rpc.user.UserSession;
+import org.junit.Test;
+
+import java.net.SocketAddress;
+import java.util.concurrent.CountDownLatch;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+public class WebSessionResourcesTest {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(WebSessionResourcesTest.class);
+
+  private WebSessionResources webSessionResources;
+
+  private boolean listenerComplete;
+
+  private CountDownLatch latch;
+
+  private EventExecutor executor;
+
+
--- End diff --

Sure will add a comment here. The purpose of this test was to validate that 
with Null channel in closeFuture which is used by WebSessionResources, during 
`WebSessionResources:close` it works as expected.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145566366
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebSessionResources.java
 ---
@@ -68,16 +68,19 @@ public SocketAddress getRemoteAddress() {
 
   @Override
   public void close() {
-
 try {
   AutoCloseables.close(webUserSession, allocator);
 } catch (Exception ex) {
   logger.error("Failure while closing the session resources", ex);
 }
 
-// Set the close future associated with this session.
+// Notify all the listeners of this closeFuture for failure events so 
that listeners can do cleanup related to this
+// WebSession. This will be called after every query execution by 
AnonymousWebUserConnection::cleanupSession and
--- End diff --

From my understanding session is tied to a cookie not a connection. In case 
of anonymous session, we call close after completion of each query. But for 
authenticated session the close will be called when HttpSession is invalidated 
which is during logout or session timeout.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

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

https://github.com/apache/drill/pull/993#discussion_r145495729
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -204,9 +217,15 @@ public WebUserConnection provide() {
 
config.getLong(ExecConstants.HTTP_SESSION_MEMORY_RESERVATION),
 config.getLong(ExecConstants.HTTP_SESSION_MEMORY_MAXIMUM));
 
+// Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+// listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+// listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
+// all the running queries for this Web Client.
--- End diff --

We are using [web 
session](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java#L193)
 concept for Drill's WebServer.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144930316
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebUserConnection.java
 ---
@@ -124,6 +124,13 @@ public void sendData(RpcOutcomeListener listener, 
QueryWritableBatch result
 }
   }
 
+  /**
+   * Returns a DefaultChannelPromise which doesn't have reference to any 
actual channel but has an EventExecutor
+   * associated with it. In this case we use EventExecutor out of 
BitServer EventLoopGroup. Since there is no actual
+   * connection established using this class, hence the close event will 
never be fired by underlying layer and close
+   * future is set only when the WebSessionResources are closed.
--- End diff --

Can we take this comment up a level? This is a description of the 
mechanics, which is fine. But, what is the intent?

Are we using a web session as a pseudo-connection (as described in earlier 
comments)? Is this an adapter to make the session look like a connection?


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144930573
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/WebSessionResourcesTest.java
 ---
@@ -0,0 +1,146 @@
+/*
+ * 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.server.rest;
+
+import io.netty.channel.ChannelPromise;
+import io.netty.channel.DefaultChannelPromise;
+import io.netty.util.concurrent.EventExecutor;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.drill.exec.rpc.TransportCheck;
+import org.apache.drill.exec.rpc.user.UserSession;
+import org.junit.Test;
+
+import java.net.SocketAddress;
+import java.util.concurrent.CountDownLatch;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+public class WebSessionResourcesTest {
+  //private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(WebSessionResourcesTest.class);
+
+  private WebSessionResources webSessionResources;
+
+  private boolean listenerComplete;
+
+  private CountDownLatch latch;
+
+  private EventExecutor executor;
+
+
--- End diff --

Maybe a comment to explain what this test is, in fact, testing? If the 
mechanism itself is explained elsewhere, simply reference that with a \@see or 
\@link tag.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144929270
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -260,8 +282,14 @@ public WebUserConnection provide() {
 logger.trace("Failed to get the remote address of the http session 
request", ex);
   }
 
-  final WebSessionResources webSessionResources = new 
WebSessionResources(sessionAllocator,
-  remoteAddress, drillUserSession);
+  // Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+  // listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+  // listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
--- End diff --

Again: connection or session?


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144929150
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -204,9 +217,15 @@ public WebUserConnection provide() {
 
config.getLong(ExecConstants.HTTP_SESSION_MEMORY_RESERVATION),
 config.getLong(ExecConstants.HTTP_SESSION_MEMORY_MAXIMUM));
 
+// Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+// listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+// listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
+// all the running queries for this Web Client.
+final ChannelPromise closeFuture = new DefaultChannelPromise(null, 
executor);
+
 // Create a WebSessionResource instance which owns the lifecycle 
of all the session resources.
-// Set this instance as an attribute of HttpSession, since it will 
be used until session is destroyed.
-webSessionResources = new WebSessionResources(sessionAllocator, 
remoteAddress, drillUserSession);
+// Set this instance as an attribute of HttpSession, since it will 
be used until session is destroyed
+webSessionResources = new WebSessionResources(sessionAllocator, 
remoteAddress, drillUserSession, closeFuture);
--- End diff --

The code suggests we are using a *session*, not *connection* as mentioned 
in the earlier comment.


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144928933
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
 ---
@@ -204,9 +217,15 @@ public WebUserConnection provide() {
 
config.getLong(ExecConstants.HTTP_SESSION_MEMORY_RESERVATION),
 config.getLong(ExecConstants.HTTP_SESSION_MEMORY_MAXIMUM));
 
+// Create a dummy close future which is needed by Foreman only. 
Foreman uses this future to add a close
+// listener to known about channel close event from underlying 
layer. We use this future to notify Foreman
+// listeners when the Web connection between Web Client and 
WebServer is closed. This will help Foreman to cancel
+// all the running queries for this Web Client.
--- End diff --

Classic Web communication is connectionless and stateless. For performance, 
some browsers use a keep-alive mechanism. But, this must be entirely optional. 
In this way, an HTTP connection is completely unlike an RPC connection.

Given that the HTTP connection will be immediately closed after submitting 
a query, do we really want to kill the query in response?

Or, are we using a web session concept that "closes" in response to a 
session timeout?


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/993#discussion_r144929812
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebSessionResources.java
 ---
@@ -68,16 +68,19 @@ public SocketAddress getRemoteAddress() {
 
   @Override
   public void close() {
-
 try {
   AutoCloseables.close(webUserSession, allocator);
 } catch (Exception ex) {
   logger.error("Failure while closing the session resources", ex);
 }
 
-// Set the close future associated with this session.
+// Notify all the listeners of this closeFuture for failure events so 
that listeners can do cleanup related to this
+// WebSession. This will be called after every query execution by 
AnonymousWebUserConnection::cleanupSession and
--- End diff --

If a session is anonymous, is the session tied to a cookie (i.e. a 
browser?) If not, then does the session end on the close of the connection? If 
so, would we kill a query as soon as the web connection that submits the query 
is closed?

We might need a general comment about how this works (if one does not 
already exist.)


---

[GitHub] drill pull request #993: DRILL-5874: NPE in AnonWebUserConnection.cleanupSes...

[sohami]

GitHub user sohami opened a pull request:

https://github.com/apache/drill/pull/993

DRILL-5874: NPE in AnonWebUserConnection.cleanupSession()



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/sohami/drill DRILL-5874

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/993.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 #993


commit f549f07bcc56f8978fa5745c52ffb4e68f67f419
Author: Sorabh Hamirwasia 
Date:   2017-10-11T22:57:21Z

DRILL-5874: NPE in AnonWebUserConnection.cleanupSession()




---