cassandra-pr mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Date Wed, 06 May 2020 12:41:30 GMT

adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r420736376



##########
File path: test/unit/org/apache/cassandra/index/internal/CustomCassandraIndex.java
##########
@@ -94,6 +95,21 @@ protected boolean supportsOperator(ColumnMetadata indexedColumn, Operator
operat
     {
         return operator.equals(Operator.EQ);
     }
+    
+    public boolean supportsLoad(Loads load)
+    {
+        switch (load)
+        {
+            case ALL:
+                return supportedLoads.equals(Loads.ALL);

Review comment:
       Same as before, we can use `==`

##########
File path: src/java/org/apache/cassandra/index/internal/CassandraIndex.java
##########
@@ -146,6 +146,22 @@ protected abstract ByteBuffer getIndexedValue(ByteBuffer partitionKey,
                                                   CellPath path,
                                                   ByteBuffer cellValue);
 
+    
+    public boolean supportsLoad(Loads load)
+    {
+        switch (load)
+        {
+            case ALL:
+                return supportedLoads.equals(Loads.ALL);

Review comment:
       Being an enum, we can use `==` instead of `equals`

##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -136,6 +135,7 @@
  */
 public interface Index
 {
+    public static enum Loads {READS, WRITES, ALL, NONE};

Review comment:
       Using a plural name for a enum feels a little bit odd to me, what about something like
`LoadType` or the more operation `OperationType`? Also I can't think of other future values
for the enum given it's current usage, so I'm wondering if we could replace it by two booleans,
implicit through a pair of `supportsWrites`/`supportsReads` methods. WDYT?

##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -1126,17 +1165,26 @@ public UpdateTransaction newUpdateTransaction(PartitionUpdate update,
WriteConte
     {
         if (!hasIndexes())
             return UpdateTransaction.NO_OP;
-
-        Index.Indexer[] indexers = indexes.values().stream()
-                                          .map(i -> i.indexerFor(update.partitionKey(),
-                                                                 update.columns(),
-                                                                 nowInSec,
-                                                                 ctx,
-                                                                 IndexTransaction.Type.UPDATE))
-                                          .filter(Objects::nonNull)
-                                          .toArray(Index.Indexer[]::new);
-
-        return indexers.length == 0 ? UpdateTransaction.NO_OP : new WriteTimeTransaction(indexers);
+        
+        ArrayList<Index.Indexer> idxrs = new ArrayList<>();

Review comment:
       Ignorable nit: perhaps we could name this variable `indexers`, and use `idxrs` for
the array below. Also, I wonder if it would make sense to use an iterable instead of an array
in `WriteTimeTransaction`, so we can directly pass this `ArrayList` without the `toArray`
conversion. Would it hurt performance?

##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -424,16 +427,47 @@ public static String getIndexName(String cfName)
         assert isIndexColumnFamily(cfName);
         return StringUtils.substringAfter(cfName, Directories.SECONDARY_INDEX_NAME_SEPARATOR);
     }
+    
+    /**
+     * Does a blocking full rebuild/reovery of the specifed indexes from all the sstables
in the base table.
+     * Note also that this method of (re)building/recovering indexes:
+     * a) takes a set of index *names* rather than Indexers
+     * b) marks existing indexes removed prior to rebuilding
+     * c) fails if such marking operation conflicts with any ongoing index builds, as full
rebuilds cannot be run
+     * concurrently
+     *
+     * @param indexNames the list of indexes to be rebuilt
+     * @param isRecovery True if want to run recovery rather than rebuilding 
+     */
+    private void rebuildIndexesBlocking(Set<String> indexNames, boolean isRecovery)
+    {
+        try (ColumnFamilyStore.RefViewFragment viewFragment = baseCfs.selectAndReference(View.selectFunction(SSTableSet.CANONICAL));
+             Refs<SSTableReader> allSSTables = viewFragment.refs)
+        {
+            Set<Index> toRebuild = indexes.values().stream()
+                                          .filter(index -> indexNames.contains(index.getIndexMetadata().name))
+                                          .filter(Index::shouldBuildBlocking)
+                                          .collect(Collectors.toSet());
+            if (toRebuild.isEmpty())
+            {
+                logger.info("No defined indexes with the supplied names: {}", Joiner.on(',').join(indexNames));
+                return;
+            }
+
+            buildIndexesBlocking(allSSTables, toRebuild, true, isRecovery);
+        }
+    }
 
     /**
-     * Performs a blocking (re)indexing of the specified SSTables for the specified indexes.
+     * Performs a blocking (re)indexing/recovery of the specified SSTables for the specified
indexes.
      *
      * @param sstables      the SSTables to be (re)indexed
      * @param indexes       the indexes to be (re)built for the specifed SSTables
      * @param isFullRebuild True if this method is invoked as a full index rebuild, false
otherwise
+     * @param isFullRebuild True if this method is invoked as an index rebuild or recovery

Review comment:
       ```suggestion
        * @param isRecovery {@code true} if this method is invoked as an index rebuild or
recovery
   ```

##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -424,16 +427,47 @@ public static String getIndexName(String cfName)
         assert isIndexColumnFamily(cfName);
         return StringUtils.substringAfter(cfName, Directories.SECONDARY_INDEX_NAME_SEPARATOR);
     }
+    
+    /**
+     * Does a blocking full rebuild/reovery of the specifed indexes from all the sstables
in the base table.

Review comment:
       ```suggestion
        * Does a blocking full rebuild/recovery of the specifed indexes from all the sstables
in the base table.
   ```




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



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


Mime
View raw message