New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-21445 New implementation for local IndexQuery #11317
base: master
Are you sure you want to change the base?
Conversation
|
||
GridDhtCacheAdapter<?, ?> cacheAdapter = cctx.isNear() ? cctx.near().dht() : cctx.dht(); | ||
|
||
Set<Integer> lostParts = cacheAdapter.topology().lostPartitions(); | ||
|
||
if (!lostParts.isEmpty()) { | ||
if (part == null || lostParts.contains(part)) { | ||
throw new CacheException(new CacheInvalidStateException("Failed to execute query because cache partition " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -607,7 +611,7 @@ private <R> CacheQueryFuture<R> execute0(@Nullable IgniteReducer<T, R> rmtReduce | |||
} | |||
} | |||
|
|||
return new GridEmptyCloseableIterator(); | |||
throw new IgniteException(new ClusterGroupEmptyException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you change the behavior here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this kind of Exception is not thrown here then the IndexQueryLocalTest#testClientNodeReplicatedCache() will fail.
@@ -818,8 +840,8 @@ private QueryCursor<Cache.Entry<K, V>> queryContinuous(AbstractContinuousQuery q | |||
return (FieldsQueryCursor<R>)ctx.kernalContext().query().querySqlFields(ctx, (SqlFieldsQuery)qry, | |||
null, keepBinary, true).get(0); | |||
|
|||
if (qry instanceof ScanQuery) | |||
return query((ScanQuery)qry, null, projection(qry.isLocal())); | |||
if (qry instanceof ScanQuery || (qry instanceof IndexQuery && qry.isLocal())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like if
for IndexQuery
can be moved inside the call on the 846L.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
p, transformer, scanQry.getPartition(), isKeepBinary, qry.isLocal(), null); | ||
} | ||
else { | ||
IndexQuery idxQryLoc = (IndexQuery)qry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is already specified in IgniteCacheProxyImpl#qyery(Query, ClusterGroup)
. Let's move handle of IndexQuery in the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -625,7 +629,7 @@ private <R> CacheQueryFuture<R> execute0(@Nullable IgniteReducer<T, R> rmtReduce | |||
GridCloseableIterator it; | |||
|
|||
if (loc) | |||
it = qryMgr.scanQueryLocal(this, true); | |||
it = qryMgr.queryLocal(this, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks strange for non-local query we call scanQueryDistributed
while for local it calls generic method. Let's separate scanQueryLocal
and indexQueryLocal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* | ||
* @param qry Query. | ||
* @param updateStatistics Update statistics flag. | ||
*/ | ||
@SuppressWarnings({"unchecked"}) | ||
protected GridCloseableIterator scanQueryLocal(final GridCacheQueryAdapter qry, | ||
protected GridCloseableIterator queryLocal(final GridCacheQueryAdapter qry, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create 2 methods - scanQueryLocal
and indexQueryLocal
. They should reuse some common code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
SPI, | ||
|
||
/** Index query. */ | ||
INDEX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like now we need a test for new QueryType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added, new functionality for recording index query events added.
...e/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryManager.java
Outdated
Show resolved
Hide resolved
EVT_CACHE_QUERY_EXECUTED, | ||
CacheQueryType.SCAN.name(), | ||
qry.type() == SCAN ? CacheQueryType.SCAN.name() : CacheQueryType.INDEX.name(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qry.type().name()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
namex, | ||
null, | ||
clsName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this code was used for scan queries only, so the class name was irrelevant for cache query events and therefore was always ‘null’. Now this code processes both scan and index queries, and with index queries we need to record the name of the queried class to the event.
...e/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryManager.java
Outdated
Show resolved
Hide resolved
return scanIterator(qry, transformer, true); | ||
} | ||
catch (Exception e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you change exception class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of 'try-catch' in scanQueryLocal and indexQueryLocal by using IgniteThrowableSupplier.
…rtition checks added + refactoring
…to one sequence of methods
…l#query(Query, ClusterGroup) + separate methods for local Scan and Index Queries
…tion correction, minor changes
…ording Index Query events added
…s moved to a different patch
…dCacheQueryManager
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summary
whereXXXX
- number of JIRA issue.(see the Maintainers list)
the
green visa
attached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.