ISPN-6395 Unify clustered queries with non clustered queries #5600

gustavocoding · 2017-11-21T21:31:45Z

https://issues.jboss.org/browse/ISPN-6395

~~Preview, just to gather feedback (from @anistor mainly) on the backwards compatible API changes.~~

This PR introduces a IndexQueryQueryMode that specifies how an indexed query is executed. BROADCAST mean the query is sent to all nodes (and results aggregated), while ~~CALLER~~ FETCH (the default) executes the query directly (thus needs a distributed index to work).

Why is this relevant? Because with BROADCAST, each node can index its own data on its own index, so both indexing and querying is way more scalable than having a single global index handled by the InfinispanIndexManager

gustavocoding · 2017-12-05T20:02:46Z

Updated

tristantarrant

Minimal stuff

tristantarrant · 2017-12-05T20:09:17Z

server/rest/src/test/java/org/infinispan/rest/search/SingleNodeLocalIndexTest.java

+      ConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+      configurationBuilder.clustering().cacheMode(CacheMode.LOCAL);
+      configurationBuilder.indexing().index(Index.ALL)
+            .addProperty("default.directory_provider", "ram");


s/ram/local-heap/

Muscle memory :)

tristantarrant · 2017-12-05T20:14:39Z

query-dsl/src/main/java/org/infinispan/query/dsl/IndexedQueryMode.java

+ *
+ * @since 9.2
+ */
+public enum IndexedQueryMode {


Shouldn't we also have an AUTO mode which chooses the best strategy depending on the underlying indexing mode ?

👍 I was planning to add it later (another PR)

+1 for introducing an AUTO mode. I think that mode should become our default. Do we actually need a value called DEFAULT?

I think the one we now call DEFAULT should be named differently. We need to find a better name that describes in a word that: 'it is executed in a single phase in the caller'. What is the opposite of clustered/distributed? Can't find a good name right now. :)

And then we could also add a DEFAULT enum value, for convenience, that is just an alias to one of the other constants (and warn users that the DEFAULT might be different in next major ispn version, as we might invent new query mechanisms). :)

AUTO will come in a later PR, I possibly will be able to squeeze it in before the year ends.

Suggestions for renaming DEFAULT ?

SINGLE_PHASE

SINGLE_STEP

LOCAL

NON_BROADCAST :)

CALLER

I've been thinking and all the names above have issues, apart for NON_BROADCAST, but this is silly. I'll go with FETCH which I believe reflects accurately the fact that the query is run locally and will read the index, potentially from remote nodes.

gustavocoding · 2017-12-06T09:04:31Z

Updated again. I removed one commit which was just about refactoring the REST search related tests, as broadcast support for REST is not 100% yet.

gustavocoding · 2017-12-06T09:57:25Z

updated one more time. Removed a "implements Serializable" left behind.

anistor · 2017-12-06T14:39:52Z

I'll have a look again also.

gustavocoding · 2017-12-06T18:09:24Z

CI seems unstable. Triggering another build

anistor · 2017-12-06T19:00:56Z

query-dsl/src/main/java/org/infinispan/query/dsl/QueryFactory.java

+    * Creates a Query based on an Ickle query string
+    * @param queryMode the {@link IndexedQueryMode} dictating the indexed query execution mode if applicable.
+    */
+   Query create(String queryString, IndexedQueryMode queryMode);


I'm looking for the way to build a QueryBuilder-DSL based query and also specify IndexedQueryMode but could not find it. Maybe I'm missing something.

And another thing, I'm not sure whether we need to specify IndexedQueryMode at query creation time; maybe it's better to specify it at execution time.

I did not touch the DSL, I though it'd be in maintenance mode only?

WRT IndexedQueryMode at execution time, I've chosen at creation time because:

It's how it's working now: SearchManager.getQuery vs SearchManager.getClusteredQuery

I tried to avoid having to change the API everywhere, i.e., query.list(mode), query.iterator(mode), query.getResultSize(mode) for each of the query types

gustavocoding · 2017-12-06T19:58:38Z

CI is green!

anistor · 2017-12-07T16:58:46Z

query/src/main/java/org/infinispan/query/QueryDefinition.java

+      return firstResult;
+   }
+
+   public void sort(Sort sort) {


setSort? to match with getSort.

anistor · 2017-12-07T16:59:42Z

query/src/main/java/org/infinispan/query/QueryDefinition.java

+      return hsQuery;
+   }
+
+   public void setMaxResults(int maxResults) {


I'd keep getter-setter pairs like getMaxResults/setMaxResults and getFirstResult/setFirstResult together without mising them with other methods like sort.

gustavocoding · 2017-12-07T17:01:11Z

Updated again. Exposed the IndexedQueryMode for Rest and Hot Rod. Did not change previous commits, so the reviews done so far are still valid!

anistor · 2017-12-07T17:02:42Z

query/src/main/java/org/infinispan/query/QueryDefinition.java

+
+   public HSQuery getHsQuery(AdvancedCache<?, ?> cache) {
+      if (hsQuery == null) {
+         QueryEngine queryEngine = cache.getComponentRegistry().getComponent(EmbeddedQueryEngine.class);


Would it be possible to pass the EmbeddedQueryEngine somehow to QueryDefinition instead of grabbing it from the component registry? Maybe in the QueryDefinition(String queryString) constructor.

Unfortunately no, the QueryDefinition is what is broadcast, so the serialization layers is responsible to construct it.

anistor · 2017-12-07T17:04:24Z

Ok. I'm still looking here at some aspects. Please do not merge it yet.

gustavocoding · 2017-12-08T11:17:37Z

Addressed reviews, changed IndexedQueryMode.DEFAULT to IndexedQueryMode.FETCH and updated documentation.

anistor · 2017-12-08T16:27:48Z

query/src/main/java/org/infinispan/query/clustered/ClusteredCacheQueryImpl.java

      return super.maxResults(maxResults);
   }

   @Override
   public CacheQuery<E> firstResult(int firstResult) {
      this.firstResult = firstResult;
+      this.queryDefinition.setFirstResult(firstResult);
      return this;
   }

   @Override
   public CacheQuery<E> sort(Sort sort) {
      this.sort = sort;


Is the sort field used at all? I see it being set but don't see it used anywhere.

sort is used in the ctor of DistributedIterator

My bad, was looking at the wrong branch. sort is unused, I'll remove it

anistor · 2017-12-08T17:07:13Z

query/src/main/java/org/infinispan/query/QueryDefinition.java

+   }
+
+   public void setNamedParameters(Map<String, Object> params) {
+      if (params != null) params.forEach(this.namedParameters::put);


Is forEach better that putAll? I suppose we also need to take care of the case when params are null.

if (params == null) { namedParameters.clear(); } else { namedParameters.putAll(params); }

anistor · 2017-12-08T17:17:29Z

query/src/main/java/org/infinispan/query/dsl/embedded/impl/QueryEngine.java

+      if (parsingResult.hasGroupingOrAggregations()) {
+         throw log.groupAggregationsNotSupported();
+      }
+      LuceneQueryParsingResult luceneParsingResult = transformParsingResult(parsingResult, EMPTY_MAP);


Replacing EMPTY_MAP with emptyMap() spares of a generics warning.

gustavocoding · 2017-12-09T09:03:28Z

Updated

anistor · 2017-12-12T08:24:36Z

...nt/hotrod-client/src/test/java/org/infinispan/client/hotrod/query/RemoteQueryStringTest.java

@@ -97,10 +97,14 @@ protected ModelFactory getModelFactory() {
      return cache;
   }

+   protected int getNodesCount() {


I don't think this is used. The derived class overrides createCacheManagers anyway.

I see, let me sort this out

anistor · 2017-12-12T11:04:29Z

query/src/main/java/org/infinispan/query/clustered/AbstractQueryDefinitionExternalizer.java

+         output.writeUTF(object.getQueryString().get());
+      } else {
+         output.writeBoolean(false);
+         output.writeObject(object.getHsQuery(null));


I don't understand how this can work with null cache parameter.

ops, let me clean this up

gustavocoding · 2017-12-12T13:50:12Z

addressed latest reviews

anistor · 2017-12-13T08:25:14Z

query/src/main/java/org/infinispan/query/QueryDefinition.java

+   }
+
+   public HSQuery getHsQuery() {
+      return hsQuery;


Calling this method should throw an IllegalStateException if hsQuery is null, ie initialize(...) was not called prior to this.

anistor · 2017-12-13T08:27:15Z

query/src/main/java/org/infinispan/query/impl/CacheQueryImpl.java

@@ -57,11 +60,26 @@ public CacheQueryImpl(Query luceneQuery, SearchIntegrator searchFactory, Advance
            cache, keyTransformationHandler);
   }

+   public CacheQueryImpl(Query luceneQuery, SearchIntegrator searchFactory, AdvancedCache<?, ?> cache,


One of the constructors in this class is no longer used.

anistor · 2017-12-13T08:38:49Z

server/rest/src/main/java/org/infinispan/rest/search/InfinispanSearchRequest.java

@@ -59,10 +61,12 @@ private QueryRequest getQueryRequest() throws IOException {
      if (request.method() == HttpMethod.GET) {
         String queryString = getParameterValue(QUERY_STRING);


The whole body of this ifstatement would look better if extracted as a separate getQueryFromString method in the spirit of getQueryFromJSON

anistor · 2017-12-13T10:07:41Z

query/src/main/java/org/infinispan/query/clustered/ClusteredQueryCommandType.java

-   public ClusteredQueryCommandWorker getCommand(Cache<?, ?> cache, HSQuery query, UUID lazyQueryId,
-            int docIndex) {
+   public ClusteredQueryCommandWorker getCommand(Cache<?, ?> cache, QueryDefinition queryDefinition, UUID lazyQueryId,
+                                                 int docIndex) {
      ClusteredQueryCommandWorker command = null;


Declaration and initialization of command can be done in same line.

anistor · 2017-12-13T15:52:17Z

query/src/main/java/org/infinispan/query/dsl/embedded/impl/QueryEngine.java

+      } else {
+         queryDefinition.initialize(cache);
+         HSQuery hsQuery = queryDefinition.getHsQuery();
+         CacheQuery cacheQuery = new CacheQueryImpl<>(hsQuery, cache, keyTransformationHandler);


These two line can become return new CacheQueryImpl<E>(hsQuery, cache, keyTransformationHandler); to avoid the warning.

anistor · 2017-12-13T16:18:03Z

I'm happy with this refactoring. I can still spot some small design issues that we can fix later.

The existence of RemoteQueryDefinition and HsQueryRequest seems to be a symptom of misplaced responsibility. It all starts with QueryDefinition.initialize, which IMO should actually be placed inside QueryEngine, not QueryDefinition. Doing that refactoring will remove the need for RemoteQueryDefinition, which now exists just to differentiate between embedded and remote case, but that differentiation can be done inside the query engine itself. Also, HsQueryRequest is just a data holder that carries the return value of QueryEngine.createHsQuery. If QueryDefinition.initialize is moved to QueryEngine we would also not need this anymore.

I did not think about it in detail but maybe we would also need to make QueryDefinition mutable for QueryEngine and immutable for external parties. In that case we can extract QueryDefinition as an immutable interface (exposing getters only) and it's implementation class could have package local setters accessible to QueryEngine only.

But let's leave those improvements for another day. I'll merge this today as it is after you have applied the last 2-3 minor changes I suggested. Thanks @gustavonalle !

gustavocoding · 2017-12-13T16:43:08Z

Updated

gustavocoding · 2017-12-13T16:48:31Z

@anistor Created https://issues.jboss.org/browse/ISPN-8628 to further refactor it

anistor · 2017-12-13T18:15:52Z

Integrated in master. Thanks @gustavonalle !

anistor · 2017-12-13T18:21:43Z

Integrated in master. Thanks @gustavonalle !

anistor · 2017-12-14T10:06:24Z

query/src/main/java/org/infinispan/query/dsl/embedded/impl/QueryEngine.java

+      LuceneQueryParsingResult luceneParsingResult = transformParsingResult(parsingResult, nameParameters);
+      org.apache.lucene.search.Query luceneQuery = makeTypeQuery(luceneParsingResult.getQuery(), luceneParsingResult.getTargetEntityName());
+      SearchIntegrator searchFactory = getSearchFactory();
+      HSQuery hsQuery = metadata == null ? searchFactory.createHSQuery(luceneQuery) : searchFactory.createHSQuery(luceneQuery);


I believe here you intended to write HSQuery hsQuery = metadata == null ? searchFactory.createHSQuery(luceneQuery) : searchFactory.createHSQuery(luceneQuery, metadata);

Fixed this here: 29e92ee

gustavocoding added the Preview label Nov 21, 2017

gustavocoding requested a review from anistor November 21, 2017 21:34

gustavocoding force-pushed the ISPN-6395 branch from 88c95b2 to b56a134 Compare November 21, 2017 22:49

gustavocoding force-pushed the ISPN-6395 branch from b56a134 to 1a3ee49 Compare December 5, 2017 20:02

gustavocoding removed the Preview label Dec 5, 2017

tristantarrant requested changes Dec 5, 2017

View reviewed changes

gustavocoding force-pushed the ISPN-6395 branch from 1a3ee49 to 99e1ed1 Compare December 6, 2017 09:02

gustavocoding force-pushed the ISPN-6395 branch from 99e1ed1 to 40ef6f6 Compare December 6, 2017 09:56

tristantarrant approved these changes Dec 6, 2017

View reviewed changes

anistor reviewed Dec 6, 2017

View reviewed changes

gustavocoding added this to the 9.2.0.Beta2 milestone Dec 7, 2017

anistor reviewed Dec 7, 2017

View reviewed changes

gustavocoding force-pushed the ISPN-6395 branch from 133ecd9 to a21eda6 Compare December 8, 2017 11:16

anistor reviewed Dec 8, 2017

View reviewed changes

gustavocoding force-pushed the ISPN-6395 branch from a21eda6 to 11dcad8 Compare December 9, 2017 09:03

ryanemerson added the Needs Rebase label Dec 11, 2017

anistor reviewed Dec 12, 2017

View reviewed changes

ryanemerson added the Changes Suggested label Dec 12, 2017

gustavocoding force-pushed the ISPN-6395 branch from e790c49 to f926c83 Compare December 12, 2017 13:49

gustavocoding removed the Changes Suggested label Dec 12, 2017

anistor reviewed Dec 13, 2017

View reviewed changes

ryanemerson added the Changes Suggested label Dec 13, 2017

anistor reviewed Dec 13, 2017

View reviewed changes

Gustavo Fernandes added 2 commits December 13, 2017 10:10

ISPN-6395 Deprecate SearchManager.getClusteredQuery

1137013

ISPN-6395 Broadcast indexed query support for Infinispan native queries

2e55e60

gustavocoding force-pushed the ISPN-6395 branch from f926c83 to 967d456 Compare December 13, 2017 10:38

gustavocoding removed the Changes Suggested label Dec 13, 2017

gustavocoding force-pushed the ISPN-6395 branch from 967d456 to 517c9f2 Compare December 13, 2017 10:43

anistor reviewed Dec 13, 2017

View reviewed changes

anistor approved these changes Dec 13, 2017

View reviewed changes

Gustavo Fernandes added 3 commits December 13, 2017 16:41

ISPN-6395 Broadcast support for REST queries

ff056d9

ISPN-6395 Add broadcast query support for Hot Rod

a40131e

ISPN-6395 Updated documentation

6b313dd

gustavocoding force-pushed the ISPN-6395 branch from 517c9f2 to 6b313dd Compare December 13, 2017 16:42

anistor closed this Dec 13, 2017

anistor reviewed Dec 14, 2017

View reviewed changes

gustavocoding deleted the ISPN-6395 branch February 20, 2018 12:13

		@@ -59,10 +61,12 @@ private QueryRequest getQueryRequest() throws IOException {
		if (request.method() == HttpMethod.GET) {
		String queryString = getParameterValue(QUERY_STRING);

ISPN-6395 Unify clustered queries with non clustered queries #5600

ISPN-6395 Unify clustered queries with non clustered queries #5600

Conversation

gustavocoding commented Nov 21, 2017 • edited

gustavocoding commented Dec 5, 2017

tristantarrant 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

gustavocoding Dec 7, 2017 • edited

Choose a reason for hiding this comment

gustavocoding Dec 8, 2017 • edited

Choose a reason for hiding this comment

gustavocoding commented Dec 6, 2017 • edited

gustavocoding commented Dec 6, 2017

anistor commented Dec 6, 2017

gustavocoding commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anistor commented Dec 7, 2017

gustavocoding commented Dec 8, 2017

Choose a reason for hiding this comment

gustavocoding Dec 9, 2017 • edited

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

gustavocoding commented Dec 9, 2017

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

gustavocoding commented Dec 12, 2017

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

anistor commented Dec 13, 2017 • edited

gustavocoding commented Dec 13, 2017

gustavocoding commented Dec 13, 2017

anistor commented Dec 13, 2017

anistor commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gustavocoding commented Nov 21, 2017 •

edited

gustavocoding Dec 7, 2017 •

edited

gustavocoding Dec 8, 2017 •

edited

gustavocoding commented Dec 6, 2017 •

edited

gustavocoding Dec 9, 2017 •

edited

anistor commented Dec 13, 2017 •

edited