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 2022/01/28 17:16:43 UTC
[GitHub] [ignite] AMashenkov opened a new pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
AMashenkov opened a new pull request #9780:
URL: https://github.com/apache/ignite/pull/9780
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795594092
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
Indexes creation is part of `SchemaManager#createIndex`:
```
final GridH2IndexBase h2Idx = desc.createUserIndex(idxDesc, cacheVisitor);
```
This method `rollbackUserIndex` is executed iff a SQL query statement fails (try-catch block). But test halts and failes earlier in `desc.createUserIndex`.
Then, index isn't registered in table due to an exception in `FailBuildIndexConsumer`, and then it becomes an orphaned index. Then it is not cleaned along with tables in moment of a cache stop. It looks like a bug.
There is a method `IndexProcessor#destroyOrphanIndex` for such indexes. I see that it actually uses only by special command, but maybe it's possible to reuse this metod and clean such indexes on cache stop.
WDYT?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795827730
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
I think there is yet another case. I'm not sure, but it could be a situation when orphaned indexes appears after dropping a _table_ but related table cache remains. Then we need check such indexes on table creation too.
If I understand correctly, it's required to do:
1. Try destroy index on the closure failure. In case of destroying fails, Ignite should WARN user about orphaned index. Then user can destroy it manually with special command.
2. Check for orphaned indexes on table creation, due to conflicts with orphaned indexes after deletion tables in past.
3. Check that all indexes are present on table start, and if no then check orphaned indexes and destroy them. Then full rebuild index started (after node recovery).
4. Destroy index on cache stop, in case of concurrent start table and delete cache.
Please correct me, if I miss smth.
I can pick this cases up in separate ticket, or you can do it within this ticket. I have a ticket with concurrent index rebuilding and IndexQuery running, so I've been going to work within this code in near time anyway.
WDYT?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795594092
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
Indexes creation is part of `SchemaManager#createIndex`:
```
final GridH2IndexBase h2Idx = desc.createUserIndex(idxDesc, cacheVisitor);
```
This method `rollbackUserIndex` is executed iff a SQL query statement fails (try-catch block). But test halts and failes earlier in `desc.createUserIndex`.
Then, index isn't registered in table due to an exception in `FailBuildIndexConsumer`, and then it becomes an orphaned index. There is a method `IndexProcessor#destroyOrphanIndex` for such indexes. I see that it actually uses only by special command, but maybe it's possible to reuse this metod and clean such indexes on cache stop.
WDYT?
, and also not cleaned along with tables in moment of a cache stop. It looks like a bug.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795609284
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
Or, we can destroy index immediately on the closure failure. Because there is no chances to use this index later.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795745540
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
I'm not sure dropping index only on closure failure will be helpful, because index dropping operation may fail either.
e.g. due to disk outage.
I think we should destroy orphan indices on table start (on or after WAL recovery) as well, due to the index tree can't be reused by the rebuild operation.
If we can't detect and drop orphan index tree on WAL recovery synchronously, then we should drop orphans on cache destroy as well, because cache could be dropped concurrently while recovery task isn't started yet.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] AMashenkov commented on a change in pull request #9780: IGNITE-16426 Add tests for known scenarios that lead to index corruption.
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on a change in pull request #9780:
URL: https://github.com/apache/ignite/pull/9780#discussion_r795745540
##########
File path: modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2Table.java
##########
@@ -988,6 +988,8 @@ public void rollbackUserIndex(String idxName) {
GridH2IndexBase rmvIdx = tmpIdxs.remove(idxName);
assert rmvIdx != null;
+
+ rmvIdx.destroy(true);
Review comment:
I'm not sure dropping index only on closure failure will be helpful, because destroy operation may fail either.
I think we should destroy orphan indices on table start (on or after WAL recovery) as well, due to the index tree can't be reused by the rebuild operation.
If we can't detect and drop orphan index tree on WAL recovery synchronously, then we should drop orphans on cache destroy as well, because cache could be dropped concurrently while recovery task isn't started yet.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org