Re: [PR] IGNITE-26683 IndexQuery uses index before data population completes; should throw “index not found” [ignite]

["timoninmaxim (via GitHub)" <gi...@apache.org>]

timoninmaxim commented on code in PR #12412:
URL: https://github.com/apache/ignite/pull/12412#discussion_r2513951411


##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexProcessor.java:
##########
@@ -383,16 +425,35 @@ public IndexesRebuildTask idxRebuild() {
      * @return Collection of indexes for specified cache.
      */
     public Collection<Index> indexes(String cacheName) {
+        return indexes(cacheName, false);
+    }
+
+    /**
+     * Returns collection of indexes for specified cache.
+     *
+     * @param cacheName Cache name.
+     * @param skipFilling If {@code true}, indexes that are currently being initially populated are excluded
+     *     from the result; if {@code false}, all known indexes for the cache are returned.
+     * @return Collection of indexes for specified cache.
+     */
+
+    public Collection<Index> indexes(String cacheName, boolean skipFilling) {
         ddlLock.readLock().lock();
 
         try {
-            Map<String, Index> idxs = cacheToIdx.get(cacheName);
+            Map<String, Index> idxMap = cacheToIdx.get(cacheName);
 
-            if (idxs == null)
+            if (idxMap == null)
                 return Collections.emptyList();
 
-            return idxs.values();
+            Collection<Index> idxs = idxMap.values();
+
+            if (!skipFilling || fillingIdxs == null)
+                return new ArrayList<>(idxs);

Review Comment:
   No need to wrap the collection to new ArrayList. This code is for internal Ignite usage only.



##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexProcessor.java:
##########
@@ -383,16 +425,35 @@ public IndexesRebuildTask idxRebuild() {
      * @return Collection of indexes for specified cache.
      */
     public Collection<Index> indexes(String cacheName) {
+        return indexes(cacheName, false);
+    }
+
+    /**
+     * Returns collection of indexes for specified cache.
+     *
+     * @param cacheName Cache name.
+     * @param skipFilling If {@code true}, indexes that are currently being initially populated are excluded
+     *     from the result; if {@code false}, all known indexes for the cache are returned.
+     * @return Collection of indexes for specified cache.
+     */
+
+    public Collection<Index> indexes(String cacheName, boolean skipFilling) {
         ddlLock.readLock().lock();
 
         try {
-            Map<String, Index> idxs = cacheToIdx.get(cacheName);
+            Map<String, Index> idxMap = cacheToIdx.get(cacheName);
 
-            if (idxs == null)
+            if (idxMap == null)
                 return Collections.emptyList();
 
-            return idxs.values();
+            Collection<Index> idxs = idxMap.values();
+
+            if (!skipFilling || fillingIdxs == null)
+                return new ArrayList<>(idxs);
 
+            return idxs.stream().filter(idx ->

Review Comment:
   Streams are heavy, avoid using them



##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexProcessor.java:
##########
@@ -218,15 +224,43 @@ public Index createIndexDynamically(
         IndexDefinition definition,
         SchemaIndexCacheVisitor cacheVisitor
     ) {
-        Index idx = createIndex(cctx, factory, definition);
+        IndexFactory dynamicFactory = (gcctx, indexDefinition) -> {
+            Index idx = factory.createIndex(gcctx, indexDefinition);
+
+            // Under lock.
+            if (fillingIdxs == null)
+                fillingIdxs = new HashSet<>();
+            fillingIdxs.add(idx.indexDefinition().idxName());

Review Comment:
   add NL before 



##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexProcessor.java:
##########
@@ -218,15 +224,43 @@ public Index createIndexDynamically(
         IndexDefinition definition,
         SchemaIndexCacheVisitor cacheVisitor
     ) {
-        Index idx = createIndex(cctx, factory, definition);
+        IndexFactory dynamicFactory = (gcctx, indexDefinition) -> {
+            Index idx = factory.createIndex(gcctx, indexDefinition);
+
+            // Under lock.

Review Comment:
   Instead of comment use `assert ddlLock.isWriteLockedByCurrentThread();`



##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexProcessor.java:
##########
@@ -218,15 +224,43 @@ public Index createIndexDynamically(
         IndexDefinition definition,
         SchemaIndexCacheVisitor cacheVisitor
     ) {
-        Index idx = createIndex(cctx, factory, definition);
+        IndexFactory dynamicFactory = (gcctx, indexDefinition) -> {
+            Index idx = factory.createIndex(gcctx, indexDefinition);
+
+            // Under lock.
+            if (fillingIdxs == null)
+                fillingIdxs = new HashSet<>();
+            fillingIdxs.add(idx.indexDefinition().idxName());
+
+            return idx;
+        };
+
+        Index idx = createIndex(cctx, dynamicFactory, definition);

Review Comment:
   Looks like this call should be in the try-catch block also. For a visitor lambda we can use IndexName from the definition variable.



##########
modules/core/src/main/java/org/apache/ignite/internal/cache/query/index/IndexQueryProcessor.java:
##########
@@ -455,4 +455,5 @@ public static String rangeDesc(RangeIndexQueryCriterion c, String fldName, Objec
 
         return r.toString();
     }
+

Review Comment:
   Useless 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.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org