You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/05/06 17:06:21 UTC
[GitHub] [ignite] timoninmaxim opened a new pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
timoninmaxim opened a new pull request #9086:
URL: https://github.com/apache/ignite/pull/9086
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
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim closed pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
timoninmaxim closed pull request #9086:
URL: https://github.com/apache/ignite/pull/9086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r639555171
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##########
@@ -264,25 +266,7 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection<?> col) {
assert Thread.holdsLock(this);
- if (limitDisabled) {
- queue.add((Collection<R>)col);
-
- cnt.addAndGet(col.size());
- }
- else {
- if (capacity >= col.size()) {
- queue.add((Collection<R>)col);
- capacity -= col.size();
-
- cnt.addAndGet(col.size());
- }
- else if (capacity > 0) {
- queue.add(new ArrayList<>((Collection<R>)col).subList(0, capacity));
- capacity = 0;
-
- cnt.addAndGet(capacity);
- }
- }
+ queue.add((Collection<R>)col);
Review comment:
Thanks for clarification, now understood your note. You're correct that it will work safe on current state of master. It's because we don't have any reduce of filter operations on received pages. But I did it by 2 reasons:
1. In parallel activity I add MergeSort algorithm, it merging data from different pages. So there are cases when we get a data from single node, and it covers a specified limit. But actually we need another page from other nodes to compare. + Maybe there could be additional cases in future for other reduce or filtering operations.
2. I don't like idea to make `enqueue()` responsible for finishing future, as there is already a working mechanism of `onDone()` and `cancel()` based on `next()` result. Also now `enqueue()` creates a new collection from sublist in case of limit applying on input collection, there is not much overhead but it can be avoid by the same existing mechanism. I believe the less code the better. So I decided to remove this duplicated mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#issuecomment-850578907
Close this PR. Limit handling moved to PR #9081 as it provides pluggable reducer functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r638626435
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##########
@@ -264,25 +266,7 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection<?> col) {
assert Thread.holdsLock(this);
- if (limitDisabled) {
- queue.add((Collection<R>)col);
-
- cnt.addAndGet(col.size());
- }
- else {
- if (capacity >= col.size()) {
- queue.add((Collection<R>)col);
- capacity -= col.size();
-
- cnt.addAndGet(col.size());
- }
- else if (capacity > 0) {
- queue.add(new ArrayList<>((Collection<R>)col).subList(0, capacity));
- capacity = 0;
-
- cnt.addAndGet(capacity);
- }
- }
+ queue.add((Collection<R>)col);
Review comment:
Hi! Sorry, could you please clarify which case do you mean? How does the cancel() invoke?
If I understand you correctly, you told that empty collection (that will be inserted to the queue after limit exceed) will trigger cancel. But actually it's not true. In this case we just check that it's empty and ask for new page [1]. So nobody will send cancel().
You can run my test "GridCacheTextQueryPagesTest" from this PR on master code base - it will fail. It checks number of pages to be loaded during a query.
[1] https://github.com/apache/ignite/blob/7faa57c49118285b830fcf614a12abda65116f51/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java#L213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r638601459
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##########
@@ -264,25 +266,7 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection<?> col) {
assert Thread.holdsLock(this);
- if (limitDisabled) {
- queue.add((Collection<R>)col);
-
- cnt.addAndGet(col.size());
- }
- else {
- if (capacity >= col.size()) {
- queue.add((Collection<R>)col);
- capacity -= col.size();
-
- cnt.addAndGet(col.size());
- }
- else if (capacity > 0) {
- queue.add(new ArrayList<>((Collection<R>)col).subList(0, capacity));
- capacity = 0;
-
- cnt.addAndGet(capacity);
- }
- }
+ queue.add((Collection<R>)col);
Review comment:
Why are you remove limit tracking from here?
Seems, onDone() + cancel() methods could be called when the limit was reached.
onDone() will prevent requesting next pages (loadPage()), and cancel() will free resources on map nodes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r639522679
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##########
@@ -264,25 +266,7 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection<?> col) {
assert Thread.holdsLock(this);
- if (limitDisabled) {
- queue.add((Collection<R>)col);
-
- cnt.addAndGet(col.size());
- }
- else {
- if (capacity >= col.size()) {
- queue.add((Collection<R>)col);
- capacity -= col.size();
-
- cnt.addAndGet(col.size());
- }
- else if (capacity > 0) {
- queue.add(new ArrayList<>((Collection<R>)col).subList(0, capacity));
- capacity = 0;
-
- cnt.addAndGet(capacity);
- }
- }
+ queue.add((Collection<R>)col);
Review comment:
I wrote nothing about an empty collection.
I just noticed, that when we detect 'limit was reached' in this method,
then it looks safe to call onDone() to finish this future and prevent internalIterator().loadPage() from being called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r633530037
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryManager.java
##########
@@ -2858,7 +2859,7 @@ public int size() {
null,
false,
keepBinary,
- null).limit(limit);
+ null).limit(limit).pageSize(pageSize);
Review comment:
```suggestion
null)
.limit(limit)
.pageSize(pageSize);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r639555171
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##########
@@ -264,25 +266,7 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection<?> col) {
assert Thread.holdsLock(this);
- if (limitDisabled) {
- queue.add((Collection<R>)col);
-
- cnt.addAndGet(col.size());
- }
- else {
- if (capacity >= col.size()) {
- queue.add((Collection<R>)col);
- capacity -= col.size();
-
- cnt.addAndGet(col.size());
- }
- else if (capacity > 0) {
- queue.add(new ArrayList<>((Collection<R>)col).subList(0, capacity));
- capacity = 0;
-
- cnt.addAndGet(capacity);
- }
- }
+ queue.add((Collection<R>)col);
Review comment:
Thanks for clarification, now understood your note. You're correct that it will work safe on current state of master. It's because we don't have any reduce of filter operations on received pages. But I did it by 2 reasons:
1. In parallel activity I add MergeSort algorithm, it merging data from different pages. So there are cases when we get a data from single node, and it covers a specified limit. But actually we need another page from other nodes to compare. + Maybe there could be additional cases in future for other reduce or filtering operations.
2. I don't like idea to make `enqueue()` responsible for finishing future, as there is already a working mechanism of `onDone()` and `cancel()` based on `hasNext()` result. Also now `enqueue()` creates a new collection from sublist in case of limit applying on input collection, there is not much overhead but it can be avoid by the same existing mechanism. I believe the less code the better. So I decide remove this duplicated mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9086: IGNITE-12291 Refactoring of query limit to avoid dummy pages loading
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9086:
URL: https://github.com/apache/ignite/pull/9086#discussion_r633530037
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryManager.java
##########
@@ -2858,7 +2859,7 @@ public int size() {
null,
false,
keepBinary,
- null).limit(limit);
+ null).limit(limit).pageSize(pageSize);
Review comment:
```suggestion
null)
.limit(limit)
.pageSize(pageSize);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org