You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/05/06 12:41:30 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
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