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/04/01 12:02:14 UTC

[GitHub] [ignite] tkalkirill opened a new pull request #8962: IGNITE-14321

tkalkirill opened a new pull request #8962:
URL: https://github.com/apache/ignite/pull/8962


   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] joooger commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
joooger commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r609724864



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -103,30 +109,42 @@
                 }
             }
 
-            if (nonNull(err))
+            if (err != null)

Review comment:
       why not **else**?




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r608359746



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java
##########
@@ -205,7 +206,7 @@
     private final LinkedHashMap<UUID, SchemaProposeDiscoveryMessage> activeProposals = new LinkedHashMap<>();
 
     /** Map from a cacheId to a future indicating that there is an in-progress index rebuild for the given cache. */
-    private final ConcurrentMap<Integer, GridFutureAdapter<Void>> idxRebuildFuts = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<Integer, GridFutureAdapter<Void>> idxRebuildFuts = new ConcurrentHashMap<>();

Review comment:
       why do we need this change ?




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607547837



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java
##########
@@ -205,7 +206,7 @@
     private final LinkedHashMap<UUID, SchemaProposeDiscoveryMessage> activeProposals = new LinkedHashMap<>();
 
     /** Map from a cacheId to a future indicating that there is an in-progress index rebuild for the given cache. */
-    private final ConcurrentMap<Integer, GridFutureAdapter<Void>> idxRebuildFuts = new ConcurrentHashMap<>();
+    private final Map<Integer, GridFutureAdapter<Void>> idxRebuildFuts = new ConcurrentHashMap<>();

Review comment:
       why do we need such changes ? 




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r608361297



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java
##########
@@ -3807,4 +3802,102 @@ public void manager(SchemaOperationManager mgr) {
             this.mgr = mgr;
         }
     }
+
+    /**
+     * Preparing futures of rebuilding indexes for caches.
+     * The future for the cache will be added only if the previous one is missing or completed.
+     *
+     * @param cacheIds Cache ids.
+     * @param initVer Initial affinity topology version of the exchange.
+     * @return Cache ids for which features have not been added.
+     */
+    public Set<Integer> prepareIndexRebuildFutures(Set<Integer> cacheIds, @Nullable AffinityTopologyVersion initVer) {
+        if (!cacheIds.isEmpty()) {
+            synchronized (idxRebuildFuts) {

Review comment:
       still have no clue why do we need such sync here, can u explain ?




-- 
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] tkalkirill commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607599997



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -38,19 +38,22 @@
 import org.apache.ignite.internal.util.typedef.internal.U;
 import org.jetbrains.annotations.Nullable;
 
-import static java.util.Objects.isNull;
-import static java.util.Objects.nonNull;
-
 /**
  * Task that rebuilds indexes.
  */
 public class IndexesRebuildTask {
     /** Index rebuilding futures for caches. Mapping: cacheId -> rebuild indexes future. */
     private final Map<Integer, SchemaIndexCacheFuture> idxRebuildFuts = new ConcurrentHashMap<>();
 
-    /** Start to rebuild. */
-    public IgniteInternalFuture<?> rebuild(GridCacheContext cctx) {
-        assert nonNull(cctx);
+    /**
+     * Start to rebuild.
+     *
+     * @param cctx Cache context.
+     * @param force Force rebuild indexes.
+     * @return A future of rebuilding cache indexes.
+     */
+    @Nullable public IgniteInternalFuture<?> rebuild(GridCacheContext cctx, boolean force) {
+        assert cctx != null;
 
         if (!CU.affinityNode(cctx.localNode(), cctx.config().getNodeFilter()))

Review comment:
       I propose to leave it as it is, it seems that the completed future means that the rebuilding began and ended, which is not true.




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607536497



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -84,17 +87,20 @@
         // To avoid possible data race.
         GridFutureAdapter<Void> outRebuildCacheIdxFut = new GridFutureAdapter<>();
 
-        // An internal future for the ability to cancel index rebuilding.
-        // This behavior should be discussed in IGNITE-14321.
         IgniteLogger log = cctx.kernalContext().grid().log();
 
+        // An internal future for the ability to cancel index rebuilding.
         SchemaIndexCacheFuture intRebFut = new SchemaIndexCacheFuture(new SchemaIndexOperationCancellationToken());
-        cancelIndexRebuildFuture(idxRebuildFuts.put(cctx.cacheId(), intRebFut), log);
+
+        SchemaIndexCacheFuture prevIntRebFut = idxRebuildFuts.put(cctx.cacheId(), intRebFut);

Review comment:
       have a call, seems we can simplify here, i huge +1 for such simplification here.  




-- 
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] tkalkirill commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607719376



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -84,17 +87,20 @@
         // To avoid possible data race.
         GridFutureAdapter<Void> outRebuildCacheIdxFut = new GridFutureAdapter<>();
 
-        // An internal future for the ability to cancel index rebuilding.
-        // This behavior should be discussed in IGNITE-14321.
         IgniteLogger log = cctx.kernalContext().grid().log();
 
+        // An internal future for the ability to cancel index rebuilding.
         SchemaIndexCacheFuture intRebFut = new SchemaIndexCacheFuture(new SchemaIndexOperationCancellationToken());
-        cancelIndexRebuildFuture(idxRebuildFuts.put(cctx.cacheId(), intRebFut), log);
+
+        SchemaIndexCacheFuture prevIntRebFut = idxRebuildFuts.put(cctx.cacheId(), intRebFut);

Review comment:
       I suggest leaving it as it is, since **GridQueryProcessor#markAsRebuildNeeded** should only be called on rebuilds of all indexes and **SchemaIndexCacheVisitorImpl** can be used to add a new index in https://github.com/apache/ignite/blob/020ce49a34a92d9fe98b668725df534c3fbd404f/modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java#L1895.




-- 
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] tkalkirill commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r609856015



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -103,30 +109,42 @@
                 }
             }
 
-            if (nonNull(err))
+            if (err != null)

Review comment:
       Because an error can appear when calling the **GridQueryProcessor#markAsRebuildNeeded** above.
   




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607553577



##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/index/IndexesRebuildTaskEx.java
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.cache.index;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.cache.persistence.CacheDataRow;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexOperationCancellationToken;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.internal.util.lang.IgniteThrowableConsumer;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Extension {@link IndexesRebuildTask} for the tests.
+ */
+class IndexesRebuildTaskEx extends IndexesRebuildTask {
+    /** Consumer for cache rows when rebuilding indexes. */
+    static final Map<String, IgniteThrowableConsumer<CacheDataRow>> cacheRowConsumer = new ConcurrentHashMap<>();

Review comment:
       is it all clear with static if we call it on multi node and single jvm ?




-- 
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] tkalkirill commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607722901



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -2548,6 +2552,9 @@ private String exchangeTimingsLogMessage(String header, List<String> timings) {
 
         final Throwable err0 = err;
 
+        if (err0 != null && cleanIdxRebuildFutures)

Review comment:
       I do not want to leave a possible memory leak that can occur due to the fact that we have passed the **init** stage but done with an error due to which it will not be possible to start rebuilding the indexes.




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607520624



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -38,19 +38,22 @@
 import org.apache.ignite.internal.util.typedef.internal.U;
 import org.jetbrains.annotations.Nullable;
 
-import static java.util.Objects.isNull;
-import static java.util.Objects.nonNull;
-
 /**
  * Task that rebuilds indexes.
  */
 public class IndexesRebuildTask {
     /** Index rebuilding futures for caches. Mapping: cacheId -> rebuild indexes future. */
     private final Map<Integer, SchemaIndexCacheFuture> idxRebuildFuts = new ConcurrentHashMap<>();
 
-    /** Start to rebuild. */
-    public IgniteInternalFuture<?> rebuild(GridCacheContext cctx) {
-        assert nonNull(cctx);
+    /**
+     * Start to rebuild.
+     *
+     * @param cctx Cache context.
+     * @param force Force rebuild indexes.
+     * @return A future of rebuilding cache indexes.
+     */
+    @Nullable public IgniteInternalFuture<?> rebuild(GridCacheContext cctx, boolean force) {
+        assert cctx != null;
 
         if (!CU.affinityNode(cctx.localNode(), cctx.config().getNodeFilter()))

Review comment:
       i found that this place of change is not yours, but i suppose that return of already completed future and replace @Nullable have huge readability impact than increasing @Nullable entropy and further _!= null_ checks, what do you think ?




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607562775



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -2548,6 +2552,9 @@ private String exchangeTimingsLogMessage(String header, List<String> timings) {
 
         final Throwable err0 = err;
 
+        if (err0 != null && cleanIdxRebuildFutures)

Review comment:
       this place is unclear for me, additional flag is always evel.




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607548173



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java
##########
@@ -242,6 +243,9 @@
     /** Cache name - value typeId pairs for which type mismatch message was logged. */
     private final Set<Long> missedCacheTypes = ConcurrentHashMap.newKeySet();
 
+    /** Mapping the cache id to the {@link GridDhtPartitionsExchangeFuture#initialVersion()} for rebuilding indexes. */
+    private final Map<Integer, AffinityTopologyVersion> idxRebuildTops = new ConcurrentHashMap<>();

Review comment:
       I hope here need to be ConcurrentMap too.




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r608356959



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheDatabaseSharedManager.java
##########
@@ -1458,17 +1491,22 @@ private void rebuildIndexes(Collection<GridCacheContext> contexts, Predicate<Gri
             1  //need at least 1 index rebuilded to print message about rebuilding completion
         );
 
-        for (GridCacheContext cacheCtx : contexts) {
-            if (!rebuildCond.test(cacheCtx))
-                continue;
+        Collection<GridCacheContext> rejected = new ArrayList<>(0);

Review comment:
       you can use lazy initialization instead of this trick. 




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607563816



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java
##########
@@ -3807,4 +3802,104 @@ public void manager(SchemaOperationManager mgr) {
             this.mgr = mgr;
         }
     }
+
+    /**
+     * Preparing futures of rebuilding indexes for caches.
+     * The future for the cache will be added only if the previous one is missing or completed.
+     *
+     * @param cacheIds Cache ids.
+     * @param initVer Initial affinity topology version of the exchange.
+     * @return Cache ids for which features have not been added.
+     */
+    public Set<Integer> prepareIndexRebuildFutures(Set<Integer> cacheIds, @Nullable AffinityTopologyVersion initVer) {
+        if (!cacheIds.isEmpty()) {
+            synchronized (idxRebuildFuts) {
+                Set<Integer> alreadyPrepared = new HashSet<>();
+
+                for (Integer cacheId : cacheIds) {
+                    GridFutureAdapter<Void> prevFut = idxRebuildFuts.get(cacheId);
+
+                    if (prevFut == null || prevFut.isDone()) {
+                        if (initVer != null) {
+                            AffinityTopologyVersion prev = idxRebuildTops.put(cacheId, initVer);
+
+                            assert prev == null;
+                        }
+
+                        idxRebuildFuts.put(cacheId, new GridFutureAdapter<>());
+                    }
+                    else
+                        alreadyPrepared.add(cacheId);
+                }
+
+                return alreadyPrepared;
+            }
+        }
+        else
+            return emptySet();
+    }
+
+    /**
+     * Removing futures of rebuilding indexes that should have been rebuilt on the exchange.
+     *
+     * @param fut Exchange future.
+     * @param cacheIds Cache ids for which futures will be deleted,
+     *      if {@code null} then ids will be taken from the {@code fut}.
+     */
+    public void removeIndexRebuildFuturesOnExchange(
+        GridDhtPartitionsExchangeFuture fut,
+        @Nullable Set<Integer> cacheIds
+    ) {
+        if (cacheIds == null)
+            cacheIds = rebuildIndexCacheIds(fut);
+
+        if (!cacheIds.isEmpty()) {
+            synchronized (idxRebuildFuts) {

Review comment:
       why do we need such sync here ? 




-- 
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] joooger commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
joooger commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r609726857



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/managers/indexing/IndexesRebuildTask.java
##########
@@ -103,30 +109,42 @@
                 }
             }
 
-            if (nonNull(err))
+            if (err != null)
                 U.error(log, "Failed to rebuild indexes for cache: " + cacheName, err);
 
-            outRebuildCacheIdxFut.onDone(err);
-
             idxRebuildFuts.remove(cctx.cacheId(), intRebFut);
             intRebFut.onDone(err);
+
+            outRebuildCacheIdxFut.onDone(err);
         });
 
         startRebuild(cctx, rebuildCacheIdxFut, clo, intRebFut.cancelToken());
 
         return outRebuildCacheIdxFut;
     }
 
-    /** Actual start rebuilding. Use this method for test purposes only. */
-    protected void startRebuild(GridCacheContext cctx, GridFutureAdapter<Void> fut,
-        SchemaIndexCacheVisitorClosure clo, SchemaIndexOperationCancellationToken cancel) {
+    /**
+     * Actual start rebuilding. Use this method for test purposes only.
+     *
+     * @param cctx Cache context.
+     * @param fut rebuildIdxFut Future for rebuild indexes.

Review comment:
       ```suggestion
        * @param fut Future for rebuild indexes.
   ```




-- 
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] zstan commented on a change in pull request #8962: IGNITE-14321 Fixes for the ability to force rebuild indexes for caches

Posted by GitBox <gi...@apache.org>.
zstan commented on a change in pull request #8962:
URL: https://github.com/apache/ignite/pull/8962#discussion_r607546913



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheDatabaseSharedManager.java
##########
@@ -1429,25 +1432,55 @@ private int resolvePageSizeFromPartitionFile(Path partFile) throws IOException,
         if (defrgMgr != null)
             return;
 
-        rebuildIndexes(cctx.cacheContexts(), (cacheCtx) -> cacheCtx.startTopologyVersion().equals(exchangeFut.initialVersion()));
+        Collection<GridCacheContext> rejected = rebuildIndexes(
+            cctx.cacheContexts(),
+            (cacheCtx) -> cacheCtx.startTopologyVersion().equals(exchangeFut.initialVersion()) &&
+                cctx.kernalContext().query().rebuildIndexOnExchange(cacheCtx.cacheId()),
+            false
+        );
+
+        if (!rejected.isEmpty()) {
+            cctx.kernalContext().query().removeIndexRebuildFuturesOnExchange(
+                exchangeFut,
+                rejected.stream().map(GridCacheContext::cacheId).collect(toSet())
+            );
+        }
     }
 
     /** {@inheritDoc} */
-    @Override public void forceRebuildIndexes(Collection<GridCacheContext> contexts) {
-        contexts.forEach(ctx -> cctx.kernalContext().query().prepareIndexRebuildFuture(ctx.cacheId()));
+    @Override public Collection<GridCacheContext> forceRebuildIndexes(Collection<GridCacheContext> contexts) {
+        Set<Integer> cacheIds = contexts.stream().map(GridCacheContext::cacheId).collect(toSet());
+
+        Set<Integer> rejected = cctx.kernalContext().query().prepareIndexRebuildFutures(cacheIds, null);
+
+        if (log.isDebugEnabled()) {
+            log.debug("Preparing features of rebuilding indexes for caches on force rebuild [requested=" + cacheIds +
+                ", rejected=" + rejected + ']');
+        }
+
+        rebuildIndexes(contexts, (cacheCtx) -> !rejected.contains(cacheCtx.cacheId()), true);
 
-        rebuildIndexes(contexts, (cacheCtx) -> true);
+        return rejected.isEmpty() ? emptyList() :
+            contexts.stream().filter(ctx -> rejected.contains(ctx.cacheId())).collect(toList());
     }
 
     /**
+     * Rebuilding indexes for caches.
+     *
      * @param contexts Collection of cache contexts for which indexes should be rebuilt.
      * @param rebuildCond Condition that should be met for indexes to be rebuilt for specific cache.
+     * @param force Force rebuild indexes.
+     * @return Cash contexts that did not pass by {@code rebuildCond}.

Review comment:
       cool typo ))




-- 
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