implement cancel function #1367

Rkr1992 · 2020-06-03T22:38:23Z

Resolves #1292 (if appropriate)

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

License

I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

aklish · 2020-06-04T13:56:48Z

elide-core/src/main/java/com/yahoo/elide/core/CancelSession.java

+ * See LICENSE file in project root for terms.
+ */
+
+import org.hibernate.Session;


Elide core should not depend directly on Hibernate. Only the data stores that use hibernate should have this dependency.

aklish · 2020-06-04T14:01:36Z

elide-core/src/main/java/com/yahoo/elide/core/CancelSession.java

+ * Cancel Session implementation.
+ */
+
+public abstract class CancelSession {


Instead of creating an abstract class - create a functional interface (with no dependencies on things like hibernate).

I think the functionalInterface will require an entityManager as an argument - and as such - it should probably live in the Aggregation Store and JpaStore respectively.

aklish · 2020-06-04T14:08:36Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

@@ -376,10 +378,10 @@ default AuditLogger getAuditLogger() {
     */
    default DataStore getDataStore(MetaDataStore metaDataStore, AggregationDataStore aggregationDataStore,
            EntityManagerFactory entityManagerFactory) {
-
+	CancelSession cancelSession = new CancelSession(entityManagerFactory.get().unwrap(Session.class));


YOu can make this a local variable and use it multiple places.

Another issue - you are unwrapping the factory - not the entity manager. I don't think that is correct.

aklish · 2020-06-04T21:37:57Z

...e-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStore.java

@@ -75,6 +83,23 @@ public void populateEntityDictionary(EntityDictionary dictionary) {

    @Override
    public DataStoreTransaction beginTransaction() {
-        return new AggregationDataStoreTransaction(queryEngine);
+	EntityManager entityManager = entityManagerSupplier.get();        


Don't create an entityManager here. Let the transaction create the entity manager.

aklish · 2020-06-04T21:38:24Z

...e-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStore.java


    /**
     * These are the classes the Aggregation Store manages.
     */
    private static final List<Class<? extends Annotation>> AGGREGATION_STORE_CLASSES =
            Arrays.asList(FromTable.class, FromSubquery.class);

-    public AggregationDataStore(QueryEngine queryEngine) {
+    public AggregationDataStore(EntityManagerSupplier entityManagerSupplier, QueryEngine queryEngine, AggregationDataStoreTransactionCancel aggregationDataStoreTransactionCancel) {


Let's not add an EntityManagerSupplier to the constructor.

aklish · 2020-06-04T21:38:35Z

...e-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStore.java

+     * Functional interface for describing a method to supply EntityManager.
+     */
+    @FunctionalInterface
+    public interface EntityManagerSupplier {


aklish · 2020-06-04T21:39:14Z

...e-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStore.java

+     * Functional interface for describing a method to supply AggregationDataStoreTransaction.
+     */
+    @FunctionalInterface
+    public interface  AggregationDataStoreTransactionCancel {


Can we shorten the name of this interface (we already know this is for the AggregationDataStore since it is an inner class).

aklish · 2020-06-04T21:40:23Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreTransaction.java

-    public AggregationDataStoreTransaction(QueryEngine queryEngine) {
-        this.queryEngine = queryEngine;
+    private AggregationDataStore.AggregationDataStoreTransactionCancel aggregationDataStoreTransactionCancel;
+    public AggregationDataStoreTransaction(EntityManager em, QueryEngine queryEngine, AggregationDataStore.AggregationDataStoreTransactionCancel aggregationDataStoreTransactionCancel) {


Remove the entity manager from the constructor. The AggregationStore (other than the SQLQueryEngine) should not have a direct dependency on the EntityManager

aklish · 2020-06-04T21:44:35Z

...ore-jpa/src/main/java/com/yahoo/elide/datastores/jpa/transaction/AbstractJpaTransaction.java

        this.em = em;
        this.emWrapper = new EntityManagerWrapper(em);
+	this.jpaTransactionCancel = jpaTransactionCancel;


Why not implement cancel here so we don't have to redefine it in each subclass.

aklish · 2020-06-04T21:45:41Z

...g-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java

@@ -170,19 +171,25 @@ public DataStore buildDataStore(EntityManagerFactory entityManagerFactory, Query
            ObjectProvider<ElideDynamicEntityCompiler> dynamicCompiler, ElideConfigProperties settings)
            throws ClassNotFoundException {
        AggregationDataStore aggregationDataStore = null;
+	JpaDataStore.JpaTransactionCancel jpaTransactionCancel = null;	


This can be assigned like:

jpaTransactionCancel = (entityManager) -> { entityManager.unwrap(Session.class).cancelQuery();};

and then you can reuse jpaTransactionCancel below.

aklish · 2020-06-04T21:47:44Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

-                (em -> { return new NonJtaTransaction(em); }));
-
+		(em -> { return new NonJtaTransaction(em, em.unwrap(Session.class).cancelQuery()); }),
+                     (entityManager -> { entityManager.unwrap(Session.class).cancelQuery();}));


This doesn't look right - why are we defining the cancel method twice here and passing it through two separate functions?

aklish · 2020-06-04T22:13:56Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

@@ -393,18 +393,19 @@ default DataStore getDataStore(MetaDataStore metaDataStore, AggregationDataStore
     * @return AggregationDataStore object initialized.
     */
    default AggregationDataStore getAggregationDataStore(QueryEngine queryEngine,
-            Optional<ElideDynamicEntityCompiler> optionalCompiler) {
+            Optional<ElideDynamicEntityCompiler> optionalCompiler, EntityManagerFactory entityManagerFactory) {


Remove entityManagerFactory as an argument.

aklish · 2020-06-04T22:14:39Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

-            aggregationDataStore = new AggregationDataStore(queryEngine);
+            aggregationDataStore = new AggregationDataStore(
+            	() -> { return entityManagerFactory.createEntityManager(); },
+                queryEngine, (entityManager -> { entityManager.unwrap(Session.class).cancelQuery();}));


We define the transaction cancel method four times. Let's define this once with a static, final variable and just use that everywhere.

aklish · 2020-06-05T18:40:58Z

...-datastore-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/QueryEngine.java

@@ -163,7 +163,7 @@ private void populateMetaData(MetaDataStore metaDataStore) {
     * @param query The query customized for a particular persistent storage or storage client
     * @return query results
     */
-    public abstract QueryResult executeQuery(Query query);
+    public abstract FutureImplementation<QueryResult> executeQuery(Query query);


Just return a Future here instead of the Implementation.

aklish · 2020-06-05T18:41:39Z

...e-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/FutureImplementation.java

+
+import java.util.concurrent.Future;
+
+public abstract class FutureImplementation implements Future<QueryResult> {


You could make this an inner class of the SQLQueryEngine - not abstract - and it can just call the TransactionCancel member.

aklish · 2020-06-05T21:35:28Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+	    this.entityManagerFactory = entityManagerFactory;
+	}
+    	public boolean cancel(boolean mayInterruptIfRunning) {
+            transactionCancel.cancel(entityManagerFactory.createEntityManager());


So this line creates a new session and then immediately cancels it. We want to close the existing session - not a new one.

aklish · 2020-06-05T21:35:50Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

        super(metaDataStore, cache);
        this.entityManagerFactory = entityManagerFactory;
        this.referenceTable = new SQLReferenceTable(metaDataStore);
+	this.transactionCancel = transactionCancel;


There are several places where spacing is off.

aklish · 2020-06-05T21:36:52Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+    }
+
+    public class FutureImplementation implements Future<QueryResult> {
+        private final TransactionCancel transactionCancel;


An inner class can reference the members of the outer class if it is not static. There is no need to:

Define private members

Have a constructor with arguments.

aklish · 2020-06-05T21:38:06Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+	 * Cancels transaction
+         */
+    	@Override
+	protected FutureImplementation(EntityManagerFactory entityManagerFactory, TransactionCancel transactionCancel) {


Let's find a better name than FutureImplementation. How about QueryResultFuture?

aklish · 2020-06-05T21:38:33Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+	    this.transactionCancel = transactionCancel;
+	    this.entityManagerFactory = entityManagerFactory;
+	}
+    	public boolean cancel(boolean mayInterruptIfRunning) {


Add @Override.

aklish · 2020-06-05T21:40:26Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

@@ -60,13 +61,14 @@
 @Slf4j
 public class SQLQueryEngine extends QueryEngine {
    private final EntityManagerFactory entityManagerFactory;
-
+    private final TransactionCancel transactionCancel;


I don't see the implementation of executeQuery that returns a Future here.

aklish · 2020-06-05T21:42:10Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+	    this.transactionCancel = transactionCancel;
+	    this.entityManagerFactory = entityManagerFactory;
+	}
+    	public boolean cancel(boolean mayInterruptIfRunning) {


You are missing a lot of the other Future functions. There is more than just cancel.

coveralls · 2020-06-12T03:34:46Z

Coverage decreased (-0.1%) to 80.536% when pulling 127517b on Rkr1992:implement-cancel-function into af132fd on yahoo:elide-5.x.

aklish · 2020-06-12T15:34:55Z

elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java

@@ -267,4 +267,9 @@ default boolean supportsPagination(Class<?> entityClass, FilterExpression expres
     * @return UUID id
     */
    UUID getRequestId();
+
+    /**
+     * cancel running transaction


Can you mention in the comments that the implementation must be thread-safe?

aklish · 2020-06-12T15:35:44Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreTransaction.java

-        if (entityProjection.getPagination() != null && entityProjection.getPagination().returnPageTotals()) {
-            entityProjection.getPagination().setPageTotals(result.getPageTotals());
+        queryResult = queryEngine.executeQuery(query);
+        queryResult.run();


You don't need to call run() here. Calling queryResult.get() should invoke queryResult.run()

aklish · 2020-06-12T15:36:36Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/AggregationDataStoreTransaction.java

+            }
+            return result.getData();
+        } catch (TransactionException e) {
+            throw new TransactionException(null);


Don't set null here.

aklish · 2020-06-12T15:38:23Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

-        super(metaDataStore, cache);
-        this.entityManagerFactory = entityManagerFactory;
-        this.referenceTable = new SQLReferenceTable(metaDataStore);
+    public SQLQueryEngine(MetaDataStore mDStore, EntityManagerFactory emFactory, Cache cx, TransactionCancel txCancel) {


cache is much more readable than cx. Can you revert back to the more readable variable names?

Same for mDStore, qEngine, emFactory, etc.

aklish · 2020-06-12T15:41:21Z

...on/src/main/java/com/yahoo/elide/datastores/aggregation/queryengines/sql/SQLQueryEngine.java

+        public void cancel(EntityManager entityManager);
+    }
+
+    public class QueryResultFuture<V> extends FutureTask<V> {


This inner class is missing javadoc. The linter will complain.

aklish · 2020-06-12T15:49:47Z

...datastore/elide-datastore-jpa/src/main/java/com/yahoo/elide/datastores/jpa/JpaDataStore.java

        transaction.begin();
        return transaction;
    }

    @Override
    public DataStoreTransaction beginTransaction() {
        EntityManager entityManager = entityManagerSupplier.get();
-        JpaTransaction transaction = writeTransactionSupplier.get(entityManager);
+        JpaTransaction transaction = writeTransactionSupplier.get(entityManager, jpaTransactionCancel);


You shouldn't need to pass transactionCancel as an argument to the supplier. When the supplier is created, it can have a reference to transactionSupplier through closure. Remove this extra argument.

aklish · 2020-06-12T15:51:43Z

...re/elide-datastore-jpa/src/test/java/com/yahoo/elide/datastores/jpa/JpaDataStoreHarness.java

@@ -96,7 +99,8 @@ public JpaDataStoreHarness() {

        store = new JpaDataStore(
                () -> { return emf.createEntityManager(); },
-                (entityManager) -> { return new NonJtaTransaction(entityManager); }
+                (entityManager, txCancel) -> { return new NonJtaTransaction(entityManager, txCancel); },


So for example, there is no need to include txCancel as a parameter to the supplier. Rewrite like:

(entityManager) -> { return new NonJtaTransaction(entityManager, jpaTransactionCancel); }

aklish · 2020-06-12T15:54:01Z

...g-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java

@@ -56,6 +58,10 @@
     * @return An instance of ElideDynamicEntityCompiler.
     * @throws Exception Exception thrown.
     */
+
+     private final AbstractJpaTransaction.JpaTransactionCancel jTC = (e) -> { e.unwrap(Session.class).cancelQuery(); };


Because the signatures are the same, you may be able to define this once as a Function rather than the concrete types above - and just use the single reference below.

aklish · 2020-06-12T15:54:59Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

        DataStore jpaDataStore = new JpaDataStore(
                () -> { return entityManagerFactory.createEntityManager(); },
-                (em -> { return new NonJtaTransaction(em); }));
+                ((em, txCancel) -> { return new NonJtaTransaction(em, txCancel); }),


Remove txCancel from the supplier.

aklish · 2020-06-12T15:55:22Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

@@ -56,6 +58,10 @@
 */
 public interface ElideStandaloneSettings {
    /* Elide settings */
+
+     public final AbstractJpaTransaction.JpaTransactionCancel JTC = (m) -> { m.unwrap(Session.class).cancelQuery(); };


See if these can be consolidated into a single variable using one of Java's built in Function classes.

aklish · 2020-06-17T15:08:41Z

...datastore/elide-datastore-jpa/src/main/java/com/yahoo/elide/datastores/jpa/JpaDataStore.java

@@ -8,7 +8,7 @@
 import com.yahoo.elide.core.DataStoreTransaction;
 import com.yahoo.elide.core.EntityDictionary;
 import com.yahoo.elide.core.datastore.JPQLDataStore;
-import com.yahoo.elide.datastores.jpa.transaction.AbstractJpaTransaction;
+//import com.yahoo.elide.datastores.jpa.transaction.AbstractJpaTransaction;


Remove this comment.

aklish · 2020-06-17T15:09:57Z

...e-datastore-jpa/src/main/java/com/yahoo/elide/datastores/jpa/transaction/JtaTransaction.java

        this(entityManager, lookupUserTransaction(), jpaTransactionCancel);
    }

-    public JtaTransaction(EntityManager entityManager, UserTransaction transaction, JpaTransactionCancel txCancel) {
+    public JtaTransaction(EntityManager entityManager, UserTransaction transaction, Consumer<EntityManager> txCancel) {


Unify the variable names txCancel and jpaTransactionCancel.

aklish · 2020-06-17T15:12:43Z

...g-boot-autoconfigure/src/main/java/com/yahoo/elide/spring/config/ElideAutoConfiguration.java

@@ -59,8 +60,7 @@
     * @throws Exception Exception thrown.
     */

-     private final AbstractJpaTransaction.JpaTransactionCancel jTC = (e) -> { e.unwrap(Session.class).cancelQuery(); };
-     private final SQLQueryEngine.TransactionCancel txCancel = (em) -> { em.unwrap(Session.class).cancelQuery(); };
+     private final Consumer<EntityManager> func = (em) -> { em.unwrap(Session.class).cancelQuery(); };


txCancel was a better name than func. I would change everywhere.

aklish · 2020-06-17T15:13:33Z

elide-standalone/src/main/java/com/yahoo/elide/standalone/config/ElideStandaloneSettings.java

@@ -59,8 +59,7 @@
 public interface ElideStandaloneSettings {
    /* Elide settings */

-     public final AbstractJpaTransaction.JpaTransactionCancel JTC = (m) -> { m.unwrap(Session.class).cancelQuery(); };
-     public final SQLQueryEngine.TransactionCancel TXCANCEL = (m) -> { m.unwrap(Session.class).cancelQuery(); };
+     public final Consumer<EntityManager> FUNC = (em) -> { em.unwrap(Session.class).cancelQuery(); };


Rename to TXCANCEL

aklish · 2020-06-17T15:14:37Z

...-datastore-aggregation/src/main/java/com/yahoo/elide/datastores/aggregation/QueryEngine.java

@@ -157,6 +156,7 @@ private void populateMetaData(MetaDataStore metaDataStore) {
    public interface Transaction extends AutoCloseable {
        @Override
        void close();
+        void cancel();


Let's add javadoc for cancel (close isn't needed because it is an Override).

* implement cancel function * addressing comments * addressing comments * Future implementation * Future implementation * Future implementation * Future implementation * Future implementation * adding Future Task * adding Future Task * adding Future Task * fixing future implementation * fixing future implementation * fixing future implementation * fixing future implementation * fixing future implementation * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing issues * fixing bugs * fixing issues * fixing issues * fixing issues * fixing issues * fixing bugs * fixing bugs * fixing bugs * fixing bugs * fixing bugs * fixing bugs * fixing bugs * fixing bugs * ExecutionException * fixing issues * aggregation changes * aggregation changes * aggregation changes * fixing bugs * fixing bugs * fixing bugs * rebasing * adddressing comments * adddressing comments * addressing comments * addressing comments Co-authored-by: Ramsha Rao <ramsha.rao@verizonmedia.com>

implement cancel function

c0492ff

aklish requested changes Jun 4, 2020

View reviewed changes

Ramsha Rao added 2 commits June 4, 2020 14:59

addressing comments

a6dd1de

addressing comments

b916ca0

aklish requested changes Jun 4, 2020

View reviewed changes

Ramsha Rao added 3 commits June 5, 2020 13:23

Future implementation

de92906

Future implementation

f13304c

Future implementation

9013381

aklish requested changes Jun 5, 2020

View reviewed changes

Future implementation

8e97a51

aklish requested changes Jun 5, 2020

View reviewed changes

Ramsha Rao added 19 commits June 5, 2020 19:02

Future implementation

f08727c

adding Future Task

4e69499

adding Future Task

b69bb59

adding Future Task

65af8b2

fixing future implementation

8064be3

fixing future implementation

e34abc3

fixing future implementation

4c63c3d

fixing future implementation

4688f15

fixing future implementation

82d67de

fixing issues

c637d51

fixing issues

3ea7627

fixing issues

ffb6b1c

fixing issues

3cd0522

fixing issues

29a38cd

fixing issues

db6c4df

fixing issues

68abcb0

fixing issues

3ebdbab

fixing issues

7d139dd

fixing issues

9e0ab28

Ramsha Rao added 13 commits June 11, 2020 11:26

fixing bugs

e08e565

fixing bugs

dbc78ef

fixing bugs

e506940

fixing bugs

4cdd852

fixing bugs

86c33d3

fixing bugs

97ae6a5

ExecutionException

d54810e

fixing issues

a285916

aggregation changes

6d62363

aggregation changes

b8996a2

aggregation changes

f709787

fixing bugs

3123771

fixing bugs

bf24384

fixing bugs

43ce9e8

aklish requested changes Jun 12, 2020

View reviewed changes

Ramsha Rao and others added 5 commits June 12, 2020 18:23

rebasing

8f1cfb4

Merge branch 'elide-5.x' into implement-cancel-function

5094ce0

adddressing comments

d7556cb

adddressing comments

d6dd7bd

addressing comments

10b0803

aklish requested changes Jun 17, 2020

View reviewed changes

addressing comments

127517b

aklish approved these changes Jun 17, 2020

View reviewed changes

aklish merged commit ff86c56 into yahoo:elide-5.x Jun 17, 2020


		import java.util.concurrent.Future;

		public abstract class FutureImplementation implements Future<QueryResult> {

implement cancel function #1367

implement cancel function #1367

Conversation

Rkr1992 commented Jun 3, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 12, 2020 •

edited

Loading