From ef9d6d8429db08e5a18add8e0844d94d2ac65242 Mon Sep 17 00:00:00 2001 From: Mikhaylo Demianenko Date: Thu, 25 Aug 2016 17:04:43 +0200 Subject: [PATCH] Always add guarding operations level to have possibility to terminate transaction with custom timeouts. Allow execution of queries with custom timeout using GraphDatabaseService API. --- .../cypher/performance/PerformanceTest.scala | 2 +- .../neo4j/graphdb/GraphDatabaseService.java | 26 ++++++++++ .../org/neo4j/kernel/NeoStoreDataSource.java | 12 ++--- .../org/neo4j/kernel/guard/EmptyGuard.java | 37 -------------- .../org/neo4j/kernel/guard/TimeoutGuard.java | 5 +- .../kernel/impl/factory/DataSourceModule.java | 9 ++-- .../impl/factory/GraphDatabaseFacade.java | 13 +++++ .../impl/api/integrationtest/GuardIT.java | 51 +++---------------- .../org/neo4j/test/rule/DatabaseRule.java | 12 +++++ .../neo4j/server/database/CypherExecutor.java | 14 +---- .../kernel/ReadOnlyGraphDatabaseProxy.java | 13 +++++ 11 files changed, 84 insertions(+), 110 deletions(-) delete mode 100644 community/kernel/src/main/java/org/neo4j/kernel/guard/EmptyGuard.java diff --git a/community/cypher/cypher/src/test/scala/org/neo4j/cypher/performance/PerformanceTest.scala b/community/cypher/cypher/src/test/scala/org/neo4j/cypher/performance/PerformanceTest.scala index b2da2f6be094c..28dfbb2144499 100644 --- a/community/cypher/cypher/src/test/scala/org/neo4j/cypher/performance/PerformanceTest.scala +++ b/community/cypher/cypher/src/test/scala/org/neo4j/cypher/performance/PerformanceTest.scala @@ -66,7 +66,7 @@ class PerformanceTest extends CypherFunSuite { val t0: Double = System.nanoTime val result = db.execute("start a=node({root}) match a-->b-->c, b-->d return a,count(*)", - Collections.singletonMap("root", startPoints)) + Collections.singletonMap("root", startPoints), 0) result.resultAsString() result.close() diff --git a/community/graphdb-api/src/main/java/org/neo4j/graphdb/GraphDatabaseService.java b/community/graphdb-api/src/main/java/org/neo4j/graphdb/GraphDatabaseService.java index 517cf22d3f410..a8f339279e279 100644 --- a/community/graphdb-api/src/main/java/org/neo4j/graphdb/GraphDatabaseService.java +++ b/community/graphdb-api/src/main/java/org/neo4j/graphdb/GraphDatabaseService.java @@ -249,6 +249,7 @@ public interface GraphDatabaseService * Please ensure that any returned {@link ResourceIterable} is closed correctly and as soon as possible * inside your transaction to avoid potential blocking of write operations. * + * @param timeout transaction timeout * @return a new transaction instance */ Transaction beginTx( long timeout ); @@ -264,6 +265,19 @@ public interface GraphDatabaseService */ Result execute( String query ) throws QueryExecutionException; + /** + * Executes a query and returns an iterable that contains the result set. + * If query will not gonna be able to complete within specified timeout time interval it will be terminated. + * + * This method is the same as {@link #execute(String, java.util.Map)} with an empty parameters-map. + * + * @param query The query to execute + * @param timeout The maximum time interval within which query should be completed. + * @return A {@link org.neo4j.graphdb.Result} that contains the result set. + * @throws QueryExecutionException If the Query contains errors + */ + Result execute( String query, long timeout ) throws QueryExecutionException; + /** * Executes a query and returns an iterable that contains the result set. * @@ -274,6 +288,18 @@ public interface GraphDatabaseService */ Result execute( String query, Map parameters ) throws QueryExecutionException; + /** + * Executes a query and returns an iterable that contains the result set. + * If query will not gonna be able to complete within specified timeout time interval it will be terminated. + * + * @param query The query to execute + * @param parameters Parameters for the query + * @param timeout The maximum time interval within which query should be completed. + * @return A {@link org.neo4j.graphdb.Result} that contains the result set + * @throws QueryExecutionException If the Query contains errors + */ + Result execute( String query, Map parameters, long timeout ) throws QueryExecutionException; + /** * Registers {@code handler} as a handler for transaction events which * are generated from different places in the lifecycle of each diff --git a/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java b/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java index b68f702f668c8..fcfd8df57a55c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java @@ -48,7 +48,6 @@ import org.neo4j.kernel.api.legacyindex.AutoIndexing; import org.neo4j.kernel.configuration.Config; import org.neo4j.kernel.extension.dependency.HighestSelectionStrategy; -import org.neo4j.kernel.guard.EmptyGuard; import org.neo4j.kernel.guard.Guard; import org.neo4j.kernel.impl.api.CommitProcessFactory; import org.neo4j.kernel.impl.api.ConstraintEnforcingEntityOperations; @@ -1009,13 +1008,10 @@ private StatementOperationParts buildStatementOperations( parts = parts.override( null, null, null, lockingContext, lockingContext, lockingContext, lockingContext, lockingContext, null, null, null, null ); // + Guard - if ( !EmptyGuard.EMPTY_GUARD.equals( guard ) ) - { - GuardingStatementOperations guardingOperations = new GuardingStatementOperations( - parts.entityWriteOperations(), parts.entityReadOperations(), guard ); - parts = parts.override( null, null, guardingOperations, guardingOperations, null, null, null, null, - null, null, null, null ); - } + GuardingStatementOperations guardingOperations = new GuardingStatementOperations( + parts.entityWriteOperations(), parts.entityReadOperations(), guard ); + parts = parts.override( null, null, guardingOperations, guardingOperations, null, null, null, null, + null, null, null, null ); return parts; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/guard/EmptyGuard.java b/community/kernel/src/main/java/org/neo4j/kernel/guard/EmptyGuard.java deleted file mode 100644 index f76360695f046..0000000000000 --- a/community/kernel/src/main/java/org/neo4j/kernel/guard/EmptyGuard.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (c) 2002-2016 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Neo4j is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ -package org.neo4j.kernel.guard; - -import org.neo4j.kernel.impl.api.KernelStatement; - -public class EmptyGuard implements Guard -{ - public static final EmptyGuard EMPTY_GUARD = new EmptyGuard(); - - private EmptyGuard() - { - } - - @Override - public void check( KernelStatement statement ) - { - // empty - } -} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/guard/TimeoutGuard.java b/community/kernel/src/main/java/org/neo4j/kernel/guard/TimeoutGuard.java index c12afb51fd10f..3c9206296636e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/guard/TimeoutGuard.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/guard/TimeoutGuard.java @@ -48,7 +48,10 @@ public void check( KernelStatement statement ) private void check( KernelTransactionImplementation transaction ) { - check( getMaxTransactionCompletionTime( transaction ), "Transaction timeout." ); + if ( transaction.timeout() > 0 ) + { + check( getMaxTransactionCompletionTime( transaction ), "Transaction timeout." ); + } } private void check( long maxCompletionTime, String timeoutDescription ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/DataSourceModule.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/DataSourceModule.java index 0cda5703c9eb1..3c6b03f86cfc0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/DataSourceModule.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/DataSourceModule.java @@ -48,7 +48,6 @@ import org.neo4j.kernel.api.security.AuthSubject; import org.neo4j.kernel.builtinprocs.SpecialBuiltInProcedures; import org.neo4j.kernel.configuration.Config; -import org.neo4j.kernel.guard.EmptyGuard; import org.neo4j.kernel.guard.Guard; import org.neo4j.kernel.guard.TimeoutGuard; import org.neo4j.kernel.impl.api.NonTransactionalTokenNameLookup; @@ -159,7 +158,7 @@ public DataSourceModule( final GraphDatabaseFacadeFactory.Dependencies dependenc SchemaWriteGuard schemaWriteGuard = deps.satisfyDependency( editionModule.schemaWriteGuard ); Clock clock = getClock(); - Guard guard = createGuard( deps, config, clock, logging ); + Guard guard = createGuard( deps, clock, logging ); kernelEventHandlers = new KernelEventHandlers( logging.getInternalLog( KernelEventHandlers.class ) ); @@ -355,11 +354,9 @@ public Relationship newRelationshipProxy( long id, long startNodeId, int typeId, }; } - private Guard createGuard( Dependencies deps, Config config, Clock clock, LogService logging ) + private Guard createGuard( Dependencies deps, Clock clock, LogService logging ) { - long configuredTimeout = config.get( GraphDatabaseSettings.transaction_timeout ); - boolean isTimeoutConfigured = configuredTimeout > 0; - Guard guard = isTimeoutConfigured ? createTimeoutGuard( clock, logging ) : EmptyGuard.EMPTY_GUARD; + TimeoutGuard guard = createTimeoutGuard( clock, logging ); deps.satisfyDependency( guard ); return guard; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java index aa50d5de0c825..d12c65c4bd484 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/factory/GraphDatabaseFacade.java @@ -366,6 +366,12 @@ public Result execute( String query ) throws QueryExecutionException return execute( query, Collections.emptyMap() ); } + @Override + public Result execute( String query, long timeout ) throws QueryExecutionException + { + return execute( query, Collections.emptyMap(), timeout ); + } + @Override public Result execute( String query, Map parameters ) throws QueryExecutionException { @@ -374,6 +380,13 @@ public Result execute( String query, Map parameters ) throws Quer return execute( transaction, query, parameters ); } + @Override + public Result execute( String query, Map parameters, long timeout ) throws QueryExecutionException + { + InternalTransaction transaction = beginTransaction( KernelTransaction.Type.implicit, AccessMode.Static.FULL, timeout ); + return execute( transaction, query, parameters ); + } + // This version of execute is only needed for internal testing of LOAD CSV PERIODIC COMMIT. Can be refactored? public Result execute( InternalTransaction transaction, String query, Map parameters ) throws QueryExecutionException diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/GuardIT.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/GuardIT.java index a0f1e1048a763..3aba57088d874 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/GuardIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/GuardIT.java @@ -22,12 +22,7 @@ import org.junit.Rule; import org.junit.Test; -import java.util.Map; - import org.neo4j.graphdb.DependencyResolver; -import org.neo4j.graphdb.config.Setting; -import org.neo4j.graphdb.factory.GraphDatabaseSettings; -import org.neo4j.kernel.guard.EmptyGuard; import org.neo4j.kernel.guard.Guard; import org.neo4j.kernel.guard.TimeoutGuard; import org.neo4j.kernel.impl.api.GuardingStatementOperations; @@ -37,9 +32,7 @@ import org.neo4j.test.rule.CleanupRule; import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; -import static org.neo4j.helpers.collection.MapUtil.genericMap; public class GuardIT { @@ -47,9 +40,9 @@ public class GuardIT public CleanupRule cleanupRule = new CleanupRule(); @Test - public void timeoutGuardUsedWhenGuardEnabled() throws Exception + public void useTimeoutGuard() throws Exception { - GraphDatabaseAPI database = startDataBase( getEnabledGuardConfigMap() ); + GraphDatabaseAPI database = startDataBase(); DependencyResolver dependencyResolver = database.getDependencyResolver(); Guard guard = dependencyResolver.resolveDependency( Guard.class ); @@ -57,19 +50,9 @@ public void timeoutGuardUsedWhenGuardEnabled() throws Exception } @Test - public void emptyGuardUsedWhenGuardDisabled() throws Exception + public void includeGuardingOperationLayer() throws Exception { - GraphDatabaseAPI database = startDataBase( getDisabledGuardConfigMap() ); - - DependencyResolver dependencyResolver = database.getDependencyResolver(); - Guard guard = dependencyResolver.resolveDependency( Guard.class ); - assertThat( guard, instanceOf( EmptyGuard.class ) ); - } - - @Test - public void includeGuardingOperationLayerWhenGuardEnabled() throws Exception - { - GraphDatabaseAPI database = startDataBase( getEnabledGuardConfigMap() ); + GraphDatabaseAPI database = startDataBase(); DependencyResolver dependencyResolver = database.getDependencyResolver(); StatementOperationParts operationParts = @@ -78,33 +61,11 @@ public void includeGuardingOperationLayerWhenGuardEnabled() throws Exception assertThat( operationParts.entityWriteOperations(), instanceOf( GuardingStatementOperations.class ) ); } - @Test - public void noGuardingOperationLayerWhenGuardDisabled() throws Exception - { - GraphDatabaseAPI database = startDataBase( getDisabledGuardConfigMap() ); - - DependencyResolver dependencyResolver = database.getDependencyResolver(); - StatementOperationParts operationParts = - dependencyResolver.resolveDependency( StatementOperationParts.class ); - assertThat( operationParts.entityReadOperations(), not( instanceOf( GuardingStatementOperations.class ) ) ); - assertThat( operationParts.entityWriteOperations(), not( instanceOf( GuardingStatementOperations.class ) ) ); - } - - private GraphDatabaseAPI startDataBase( Map,String> disabledGuardConfigMap ) + private GraphDatabaseAPI startDataBase() { GraphDatabaseAPI database = - (GraphDatabaseAPI) new TestGraphDatabaseFactory().newImpermanentDatabase( disabledGuardConfigMap ); + (GraphDatabaseAPI) new TestGraphDatabaseFactory().newImpermanentDatabase(); cleanupRule.add( database ); return database; } - - private Map,String> getEnabledGuardConfigMap() - { - return genericMap( GraphDatabaseSettings.transaction_timeout, "60s" ); - } - - private Map,String> getDisabledGuardConfigMap() - { - return genericMap( GraphDatabaseSettings.transaction_timeout, "0" ); - } } diff --git a/community/kernel/src/test/java/org/neo4j/test/rule/DatabaseRule.java b/community/kernel/src/test/java/org/neo4j/test/rule/DatabaseRule.java index 56981b175fedf..c46f432317e5f 100644 --- a/community/kernel/src/test/java/org/neo4j/test/rule/DatabaseRule.java +++ b/community/kernel/src/test/java/org/neo4j/test/rule/DatabaseRule.java @@ -129,12 +129,24 @@ public Result execute( String query ) throws QueryExecutionException return getGraphDatabaseAPI().execute( query ); } + @Override + public Result execute( String query, long timeout ) throws QueryExecutionException + { + return getGraphDatabaseAPI().execute( query, timeout ); + } + @Override public Result execute( String query, Map parameters ) throws QueryExecutionException { return getGraphDatabaseAPI().execute( query, parameters ); } + @Override + public Result execute( String query, Map parameters, long timeout ) throws QueryExecutionException + { + return getGraphDatabaseAPI().execute( query, parameters, timeout ); + } + @Override public InternalTransaction beginTransaction( KernelTransaction.Type type, AccessMode accessMode ) { diff --git a/community/server/src/main/java/org/neo4j/server/database/CypherExecutor.java b/community/server/src/main/java/org/neo4j/server/database/CypherExecutor.java index ad91ffdaeab29..49607def5d169 100644 --- a/community/server/src/main/java/org/neo4j/server/database/CypherExecutor.java +++ b/community/server/src/main/java/org/neo4j/server/database/CypherExecutor.java @@ -24,7 +24,6 @@ import org.neo4j.cypher.internal.javacompat.ExecutionEngine; import org.neo4j.graphdb.DependencyResolver; -import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.kernel.GraphDatabaseQueryService; import org.neo4j.kernel.api.KernelTransaction; import org.neo4j.kernel.api.security.AccessMode; @@ -51,14 +50,12 @@ public class CypherExecutor extends LifecycleAdapter private ThreadToStatementContextBridge txBridge; private static final PropertyContainerLocker locker = new PropertyContainerLocker(); - private final boolean guardEnabled; private final Log log; public CypherExecutor( Database database, Config config, LogProvider logProvider ) { this.database = database; log = logProvider.getLog( getClass() ); - guardEnabled = config.get( GraphDatabaseSettings.transaction_timeout ) > 0; } public ExecutionEngine getExecutionEngine() @@ -93,15 +90,8 @@ public QuerySession createSession( String query, Map parameters, private InternalTransaction getInternalTransaction( HttpServletRequest request ) { - if ( guardEnabled ) - { - long customTimeout = getTransactionTimeLimit( request ); - if ( customTimeout > 0 ) - { - return beginCustomTransaction( customTimeout ); - } - } - return beginDefaultTransaction(); + long customTimeout = getTransactionTimeLimit( request ); + return customTimeout > 0 ? beginCustomTransaction( customTimeout ) : beginDefaultTransaction(); } private InternalTransaction beginCustomTransaction( long customTimeout ) diff --git a/community/shell/src/main/java/org/neo4j/shell/kernel/ReadOnlyGraphDatabaseProxy.java b/community/shell/src/main/java/org/neo4j/shell/kernel/ReadOnlyGraphDatabaseProxy.java index c6192dc3b253c..ea8a872d765a7 100644 --- a/community/shell/src/main/java/org/neo4j/shell/kernel/ReadOnlyGraphDatabaseProxy.java +++ b/community/shell/src/main/java/org/neo4j/shell/kernel/ReadOnlyGraphDatabaseProxy.java @@ -31,6 +31,7 @@ import org.neo4j.graphdb.Label; import org.neo4j.graphdb.Node; import org.neo4j.graphdb.PropertyContainer; +import org.neo4j.graphdb.QueryExecutionException; import org.neo4j.graphdb.Relationship; import org.neo4j.graphdb.RelationshipType; import org.neo4j.graphdb.ResourceIterable; @@ -115,12 +116,24 @@ public Result execute( String query ) return execute( query, Collections.emptyMap() ); } + @Override + public Result execute( String query, long timeout ) throws QueryExecutionException + { + return execute( query, Collections.emptyMap(), timeout ); + } + @Override public Result execute( String query, Map parameters ) { return readOnly(); } + @Override + public Result execute( String query, Map parameters, long timeout ) throws QueryExecutionException + { + return readOnly(); + } + @Override public Node createNode() {