OGM-1430 Support scripting stored procedures for infinispan remote (#43) #1004

ntviet18 · 2018-04-13T14:00:02Z

Hi @fax4ever @Sanne @DavideD could you please review this pull request? It's basically the same as #1000. But for Infinitispan Remote module.

fax4ever · 2018-04-13T15:04:22Z

infinispan-remote/src/test/resources/storedprocedures/resultSetProcedure.js

@@ -0,0 +1,7 @@
+// mode=local,language=javascript,parameters=[id,title]


Hi @ntviet18 . Nice work again.
Maybe we can add a different test, where the stored procedure works (read or write) with cache data. Thank you for contribution.

Maybe we can add a different test, where the stored procedure works (read or write) with cache data.

I don't think this test needs to be too complicated. The purpose of this issue is for OGM to be able to call stored procedures on Infinispan. Once you call a stored procedure and you can read the value it returns, we don't really care where the value is coming from. We might want to add an additional test in the integration test module to check for storedprocedures returning values previously inserted in the cache, but it can wait and we can make it in a separate issue.

@DavideD I created this Issue to validate a full Data integration scenario.

ntviet18 · 2018-04-16T09:18:45Z

@fax4ever Thank you. Please see the updates.

DavideD · 2018-04-16T10:06:31Z

We will integrate this pull request after #1006
Can you rebase this on top of it?

ntviet18 · 2018-04-16T10:09:48Z

@DavideD sure

DavideD · 2018-04-16T10:32:08Z

Thanks

fax4ever · 2018-04-16T12:04:31Z

With the merge of OGM-1429 Infinispan embedded stored procedures need to rebase this PR on top of master. The four commits at bottom is already present on the master.

ntviet18 · 2018-04-16T12:05:42Z

@fax4ever, I rebased

fax4ever · 2018-04-16T12:08:02Z

Thank you @ntviet18

DavideD · 2018-04-16T12:47:39Z

We are preparing a release and I think we will merge in the next one, I've started to review it though

DavideD

Very nice, but I sitll have some remarks.

Thanks a lot, we will include this in 5.4 Alpha2 probably

DavideD · 2018-04-16T12:41:15Z

core/src/main/java/org/hibernate/ogm/util/impl/TupleHelper.java

+			return tuple;
+		}
+		catch (Exception e) {
+			throw log.cannotExtractTuple( e );


The error message seems a bit vague. I think we might add some additional information if we raise the exception and the caller deal with it. After all this is a utility method. For example you could also add the name of the stored procedure that caused the exception.

DavideD · 2018-04-16T12:41:39Z

core/src/main/java/org/hibernate/ogm/util/impl/TupleHelper.java

+	}
+
+	/**
+	 * Extract tuple values from given object.


An example in the javadoc would be very helpful

DavideD · 2018-04-16T12:46:24Z

core/src/main/java/org/hibernate/ogm/util/impl/TupleHelper.java

+/**
+ * @author The Viet Nguyen &amp;ntviet18@gmail.com&amp;
+ */
+public class TupleHelper {


Some javadoc explaining that the purpose of this class is to collect methods used to convert an object into a collection of tuples would be helpful. I wonder if TuplesExtractor would be a better name. Not sure. The method could become something like extractFromMap(...), extractFromObject(...) and so on

DavideD · 2018-04-16T12:50:41Z

...va/org/hibernate/ogm/backendtck/storedprocedures/NamedParametersStoredProcedureCallTest.java

+	@SkipByGridDialect(
+			value = { INFINISPAN_REMOTE },
+			comment = "This dialects does not throw exception when using not registered parameters."
+	)


OK, but it seems it should. What happens if the parameter is not registered?

I mean this test is supposed to work for all the dialects supporting stored procedures.
By the way, what JPA says about invalid parameters? Maybe we need a specific exception. I think we need to check.

We cannot determine the invalid parameter before sending it to the execution because it's optional for Infinispan. However, we can analyze the execution result and extract the parameter name. Should we implement it?

yes, I think we should do that. Unless, it's too complicated

DavideD · 2018-04-16T12:53:00Z

core/src/test/java/org/hibernate/ogm/util/impl/TupleHelperTest.java

+/**
+ * @author The Viet Nguyen &amp;ntviet18@gmail.com&amp;
+ */
+public class TupleHelperTest {


I mentioned this before, but some javadoc with an example would help a lot. Like this is the Object and this is the expected tuple.

DavideD · 2018-04-16T12:55:33Z

...a/org/hibernate/ogm/datastore/infinispan/impl/InfinispanEmbeddedStoredProceduresManager.java

-		}
-		catch (IntrospectionException | InvocationTargetException | IllegalAccessException e) {
-			throw log.cannotExtractStoredProcedureResultSet( storedProcedureName, obj, e );
-		}


Ah ah! So it was already working like I expected initially, basically you replaced a clear error message with something vague. Please make sure to revert this change.

DavideD · 2018-04-16T13:03:19Z

...infinispanremote/test/storedprocedures/InfinispanNamedParametersStoredProcedureCallTest.java

+		StoredProcedureQuery storedProcedureQuery = em.createStoredProcedureQuery( Car.SIMPLE_VALUE_PROC, Car.class );
+		storedProcedureQuery.registerStoredProcedureParameter( 0, Integer.class, ParameterMode.IN );
+		storedProcedureQuery.setParameter( 0, 1 );
+		storedProcedureQuery.getSingleResult();


In OgmJpaTestCase there is a method called inTransaction that you can use like this:

inTransaction( em -> { StoredProcedureQuery storedProcedureQuery = em.createStoredProcedureQuery( Car.SIMPLE_VALUE_PROC, Car.class ); storedProcedureQuery.registerStoredProcedureParameter( 0, Integer.class, ParameterMode.IN ); storedProcedureQuery.setParameter( 0, 1 ); storedProcedureQuery.getSingleResult(); } );

We are trying to use it in all the new tests and this way you can remove the setup and tear down methods

DavideD · 2018-04-16T13:03:52Z

...test/java/org/hibernate/ogm/datastore/infinispanremote/utils/InfinispanRemoteTestHelper.java

+		RemoteCache<String, String> scriptCache = provider.getScriptCache();
+		scriptCache.put( Car.SIMPLE_VALUE_PROC, toResourceString( "/storedprocedures/simpleValueProcedure.js" ) );
+		scriptCache.put( Car.RESULT_SET_PROC, toResourceString( "/storedprocedures/resultSetProcedure.js" ) );
+		scriptCache.put( "exceptionalProcedure", toResourceString( "/storedprocedures/exceptionalProcedure.js" ) );


DavideD · 2018-04-16T13:05:38Z

infinispan-remote/src/test/resources/storedprocedures/simpleValueProcedure.js

@@ -0,0 +1,2 @@
+// mode=local,language=javascript
+param;


Could you add a new line at the end of the file, please?

We couldn't find a way to add the check in checkstyle but it's the way we commit code

fax4ever

Thank you @ntviet18. Could you the comments here? Thank you

fax4ever · 2018-04-16T12:40:51Z

infinispan-remote/src/test/resources/storedprocedures/resultSetProcedure.js

@@ -0,0 +1,7 @@
+// mode=local,language=javascript,parameters=[id,title]


@DavideD I created this Issue to validate a full Data integration scenario.

fax4ever · 2018-04-16T12:43:55Z

...test/java/org/hibernate/ogm/datastore/infinispanremote/utils/InfinispanRemoteTestHelper.java

@@ -158,4 +177,12 @@ public static InfinispanRemoteDatastoreProvider getProvider(SessionFactory sessi
 		}
 		return InfinispanRemoteDatastoreProvider.class.cast( provider );
 	}
+
+	private static String toResourceString(String path) {
+		return toString( InfinispanRemoteTestHelper.class.getResourceAsStream( path ) );


Maybe I miss something.. When the input stream comes closed? Thanks

thank you, I added try-with-resource in method below

fax4ever · 2018-04-16T12:46:33Z

...infinispanremote/test/storedprocedures/InfinispanNamedParametersStoredProcedureCallTest.java

+ * @author The Viet Nguyen &amp;ntviet18@gmail.com&amp;
+ */
+@TestForIssue(jiraKey = { "OGM-1430" })
+public class InfinispanNamedParametersStoredProcedureCallTest extends OgmJpaTestCase {


Maybe @RunWith(InfinispanRemoteServerRunner.class) is missing

I have checked, but somehow the factory is not created. Did I miss something?

Check you are using the right one, in this case it should be: @Runwith(InfinispanRemote**Jpa**ServerRunner.class)

fax4ever · 2018-04-16T12:59:55Z

...ava/org/hibernate/ogm/datastore/infinispanremote/impl/InfinispanRemoteDatastoreProvider.java

@@ -109,7 +115,7 @@
 	@Override
 	public void start() {
 		hotrodClient = HotRodClientBuilder.builder().withConfiguration( config, marshaller ).build();
-		hotrodClient.start();
+		scriptManager = HotRodClientBuilder.builder().withConfiguration( config, new GenericJBossMarshaller() ).build();


Maybe we can add a comment here. We need a second cache manager as workaround, as suggested here: https://issues.jboss.org/browse/ISPN-8020. When this issue will be closed, we could remove use hotrodClient.

DavideD · 2018-04-16T13:12:34Z

By the way, we also need some documentation in the manual about the fact that we can now call stored procedures with Infinispan,

ntviet18 · 2018-04-16T15:52:33Z

@DavideD @fax4ever, Thank you. I have resolved some of your comments. But could you please mark a section in the documentation where I can describe about the stored procedures?

DavideD · 2018-04-18T14:26:03Z

...a/org/hibernate/ogm/datastore/infinispan/impl/InfinispanEmbeddedStoredProceduresManager.java

@@ -59,7 +55,7 @@
 		Callable<?> callable = instantiate( storedProcedureName, className, classLoaderService );


callable doesn't say much about he content of the variable, I assume this is the storedProcedure isn't it?

It seems it is not something from this PR, so never mind

DavideD · 2018-04-18T15:15:57Z

...g/hibernate/ogm/datastore/infinispanremote/impl/InfinispanRemoteStoredProceduresManager.java

+
+	private static final Log log = LoggerFactory.make( MethodHandles.lookup() );
+
+	private static final Pattern REFERENCE_ERROR_REGEXP = Pattern.compile( ".*\"([a-zA-Z_$]+)\" is not defined in <eval>.*" );


Could you explain to me quickly what's a reference error?

Sure. We get this kind of error when we try to access an undefined variable. In our case, it's when we try to use unregistered stored procedure parameter.

DavideD · 2018-04-18T15:17:49Z

...g/hibernate/ogm/datastore/infinispanremote/impl/InfinispanRemoteStoredProceduresManager.java

+		}
+		catch (Exception e) {
+			if ( e instanceof HotRodClientException ) {
+				Matcher matcher = REFERENCE_ERROR_REGEXP.matcher( e.getMessage() );


Since you check with a regexp, we should add a test case Infinispan REmote to make sure that the error message doesn't change in future releases.

It seems that the test is present on core: NamedParametersStoredProcedureCallTest::testExceptionWhenUsingNotRegisteredParameter.

DavideD · 2018-04-18T15:54:27Z

...ava/org/hibernate/ogm/datastore/infinispanremote/impl/InfinispanRemoteDatastoreProvider.java

-		hotrodClient.start();
+		// TODO temporary create another cache manager to handle remote script registration and invocation due to incompatibility of ProtoStreamMarshaller.
+		// When https://issues.jboss.org/browse/ISPN-8020 is closed, we could remove it and reuse the common hotrodClient.
+		scriptManager = HotRodClientBuilder.builder().withConfiguration( config, new GenericJBossMarshaller() ).build();
 		config = null; //no longer needed


Are you stopping the scriptManager at some point?
Also, how come we don't need to start the hotrodClient anymore?

I believe it start by default

fax4ever · 2018-04-27T14:40:40Z

@DavideD I reviewed again the PR, if you're agree, we can merge them! Thank you again @ntviet18

DavideD · 2018-05-04T15:37:29Z

There is a new one

fax4ever reviewed Apr 13, 2018

View reviewed changes

ntviet18 force-pushed the OGM-1430-infinispan-remote-scripting branch from 5f11648 to ef474fb Compare April 16, 2018 09:14

ntviet18 force-pushed the OGM-1430-infinispan-remote-scripting branch from ef474fb to adfe2ab Compare April 16, 2018 10:35

fax4ever added the Needs rebase label Apr 16, 2018

ntviet18 force-pushed the OGM-1430-infinispan-remote-scripting branch from adfe2ab to e4c7bb2 Compare April 16, 2018 12:03

DavideD removed the Needs rebase label Apr 16, 2018

DavideD requested changes Apr 16, 2018

View reviewed changes

fax4ever reviewed Apr 16, 2018

View reviewed changes

ntviet18 force-pushed the OGM-1430-infinispan-remote-scripting branch 2 times, most recently from 0045144 to 48d66dc Compare April 16, 2018 15:46

DavideD reviewed Apr 18, 2018

View reviewed changes

OGM-1430 Support scripting stored procedures for infinispan remote (#43)

697ba6f

ntviet18 force-pushed the OGM-1430-infinispan-remote-scripting branch from 48d66dc to 697ba6f Compare April 20, 2018 08:19

ntviet18 mentioned this pull request May 4, 2018

OGM-1431 Support server task stored procedures for infinispan remote #1014

Closed

DavideD closed this May 4, 2018

		@@ -0,0 +1,7 @@
		// mode=local,language=javascript,parameters=[id,title]

		@@ -59,7 +55,7 @@
		Callable<?> callable = instantiate( storedProcedureName, className, classLoaderService );


		private static final Log log = LoggerFactory.make( MethodHandles.lookup() );

		private static final Pattern REFERENCE_ERROR_REGEXP = Pattern.compile( ".\"([a-zA-Z_$]+)\" is not defined in <eval>." );

OGM-1430 Support scripting stored procedures for infinispan remote (#43) #1004

OGM-1430 Support scripting stored procedures for infinispan remote (#43) #1004

Conversation

ntviet18 commented Apr 13, 2018

fax4ever Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

DavideD Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntviet18 commented Apr 16, 2018

DavideD commented Apr 16, 2018

ntviet18 commented Apr 16, 2018

DavideD commented Apr 16, 2018

fax4ever commented Apr 16, 2018

ntviet18 commented Apr 16, 2018

fax4ever commented Apr 16, 2018

DavideD commented Apr 16, 2018

DavideD left a 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

fax4ever left a 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

DavideD commented Apr 16, 2018

ntviet18 commented Apr 16, 2018

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

fax4ever commented Apr 27, 2018

DavideD commented May 4, 2018

fax4ever Apr 13, 2018 •

edited

Loading

DavideD Apr 16, 2018 •

edited

Loading