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/04/28 14:03:34 UTC
[GitHub] [cassandra] bereng opened a new pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
bereng opened a new pull request #570:
URL: https://github.com/apache/cassandra/pull/570
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-631559153
I have left a few suggested changes [here](https://github.com/adelapena/cassandra/commit/dc0030ea4e961a2f17ebf8cc486963eec0c1e037). Changes for the dtest are [here](https://github.com/adelapena/cassandra-dtest/commit/758508b7759dfe13af724df68ee80834662ac013).
As mentioned in my previous comment, the sequence of operations in `SIM#rebuildIndexesBlocking` is modified to:
1. Set indexes as writable to not miss incoming writes.
2. Flush the base table if some index wasn't writable before, so the potentially unindexed memtable contents are moved to the set of sstables to be indexed.
3. Acquire sstable refs and rebuild.
I have also done some modifications to the way we determine whether we should use `Index#getBuildTaskSupport` or `Index#getRecoveryTaskSupport`. `SIM#buildIndexesBlocking` is only called by either `SIM#rebuildIndexesBlocking` or `SIM#handleNotification`. I understand that if the caller is `rebuildIndexesBlocking` we are always doing a full rebuild, and probably the only reason to do that is that we are attempting to recover a damaged index. Conversely, if the caller is `handleNotification` we are doing a partial build of new data that is unable to recover previous errors. This is why I think that the `isFullRebuild` argument is enough to determine whether we are recovering or indexing new data. This makes full rebuild independent of the status of the index, so any call to `nodetool rebuild_index` will involve `getRecoveryTaskSupport`, which is intended to do any initialization work that might be needed prior to a full index build. Do you think this assumption is correct, or are we interested in treating separately full rebuilds without a previous failure?
Unit tests are also slightly modified to reflect the changes. `SecondaryIndexManagerTest` is extended with queryability/writability checks over a write-only-on-failure index.
Summarizing, I think that at this point the patch achieves the following:
* Full index rebuilds give indexes the opportunity to run specific rebuild logic, including initialization. This logic can be different from the logic that is used when regularly indexing new sstables.
* If any full rebuild or a partial build task fails, the index can start to reject queries and ignore writes until a full rebuild is successfully run. Whether it rejects queries or ignores writes is defined by the index implementation.
* Rebuilt indexes don't miss data after having been ignoring writes.
* Existing indexes preserve their original behaviour. They always accept writes and only reject reads if the initial build has failed.
* The index API is backwards compatible.
Do you think it makes any sense?
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r422050421
##########
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(LoadType load)
+ {
+ switch (load)
+ {
+ case ALL:
+ return supportedLoads == LoadType.ALL;
+ case READ:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.READ;
+ case WRITE:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.WRITE;
+ default:
+ return false;
+ }
Review comment:
Are you saying here `READ` accepts `NONE` ?
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-632103017
CI runs:
- [CI j8](https://app.circleci.com/pipelines/github/bereng/cassandra/34/workflows/aca7db2f-b52d-452d-8264-c0a478c11a76)
- [CI j11](https://app.circleci.com/pipelines/github/bereng/cassandra/34/workflows/82d75275-3e69-4c2f-b26e-4369563ecfd4)
I checked failures: SlowQueryLog is a known one, the SimpleStragey cast one is one I fixed in another ticket, the Cqlsh one on empty values passes locally, the other few look completely unrelated, etc. I _think_ we're good to merge.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r427459670
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
I think that the flush should happed right after we have ensured that the indexes are marked as writable. Otherwise, we might have new untracked writes populating the new memtable while the indexes are not writable. In the same vein, the sstable selection should happen after we have marked the indexes as writable. I have almost ready a commit with this and a few minor suggestions, I'll push it tomorrow.
We're almost there!
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-624708628
- [Ci j11](https://app.circleci.com/pipelines/github/bereng/cassandra/15/workflows/2c0458de-6534-47e6-8e7c-4909e7827001)
- [CI j8](https://app.circleci.com/pipelines/github/bereng/cassandra/15/workflows/fdab9b26-b68c-41a8-8fbe-c739fff9f37f)
^are these runs good enough or do you recommend I run any other dtests? Thx in advance
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r421980204
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -136,6 +135,7 @@
*/
public interface Index
{
+ public static enum LoadType {READ, WRITE, ALL, NONE};
Review comment:
Argh! you're SO right! My bad apologies.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r420834322
##########
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:
I think it's a personal style thing. Rather than having 2 booleans and having to reason about them, the Enum makes it more 'in your face' imo: `ALL` is clear what it means vs 2 booleans etc. :shrug:
Also maybe I have too much imagination, but I can think of 'DELETE' as a load. You could have 'append-only' indexes (think banking i.e.) in the future that don't support deletes. Maybe premature optimization is being my enemy here... Or sthg with statics for custom indexes...
I prefer you decide what is best here :-). I'll rename to singulars meanwhile in case we kept that option.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-620694006
- j11 [CI](https://app.circleci.com/pipelines/github/bereng/cassandra/11/workflows/34a10f44-be84-423b-82f7-89cbb756e8ca)
- j8 [CI](https://app.circleci.com/pipelines/github/bereng/cassandra/11/workflows/2134990d-09aa-43c2-a2bc-ef3a26d4c8e2)
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425925073
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
I'm not sure this is inline with the `LoadType.NOOP` behaviour of the indexes. Indeed, it leaves `LoadType#supportsReads()` without usages. I think that, to preserve the behaviour of the default indexes, we should make a distinction between failures in the initial build task and failures in rebuilds. As I mention on the dtest review, the classic behaviour would be:
```java
/**
* Returns the type of operations supported by the index in case its building has failed and it's needing recovery.
*
* @param isInitialBuild {@code true} if the failure is for the initial build task on index creation, {@code false}
* if the failure is for a rebuild or recovery.
*/
default LoadType getSupportedLoadTypeOnFailure(boolean isInitialBuild)
{
return isInitialBuild ? LoadType.WRITE : LoadType.ALL;
}
```
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635927896
Committed to master as [595dc61290a9fda15b6765711141039ec9609bb3](https://github.com/apache/cassandra/commit/595dc61290a9fda15b6765711141039ec9609bb3).
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r427093294
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Ah good catch! you're right that is being missed. Amazing this didn't came up until today.
In any case I'd like your feedback on the extra flush also. Feel free to shoot a half-backed push if you fancy and when I get the rest of the feedback I'll adjusts tests, dtests and that should be it hopefully.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-631991871
Hi @adelapena. Thx for the commit. I patched it and apply it here. Feel free to push here directly next time so it's authored by you rather than me so I don't steal your thunder :-)
I think what you do here makes perfect sense from the pov you have. Basically you try to infer as much as possible and rely on reasonable assumptions. I still think, call me stubborn :-) , I'd like my approach better. But as you mention this patch brings goodness and if in the future we need to move closer to my pov it's an easy change. So I am +1 on merging this. Having catched the missing flush, the extra testing around things, etc it's all good value.
Let me run CI and I'll post results here when done.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r421996369
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -1126,17 +1158,21 @@ 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<>();
+ for (Index i : indexes.values())
+ {
+ Index.Indexer idxr = i.indexerFor(update.partitionKey(), update.columns(), nowInSec, ctx, IndexTransaction.Type.UPDATE);
+ if (idxr != null && isIndexWritable(i))
Review comment:
Sgtm. That was my first implementation I can't remember for the sake of me why I changed it to a set.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r426447744
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Hi @adelapena thanks again for looking into this. You're raising the point I wanted to discuss with you once this ticket was merged: Is it reasonable a 'not ready' index should be serving reads or writes?
I think we're finding ourselves battling that, but that's a bigger discussion. Maybe in some other ticket we could recover my approach which iirc would defer to the index implementation what to do :shrug:
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r426740935
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Hi @adelapena, regarding `return isInitialBuild ? LoadType.WRITE : LoadType.ALL;`
`isInitialBuild` is a new variable we'd have to create and track in SIM. And then `Index` would have to render which load is supported depending on 'when' the failure happened?. Sounds weird to me and like we are mixing concerns. It's easier for my brain to pick the idea of having the supported load state in the `Index`, SIM to track that instead of trying to infer it, set the defaults we'd like and be done. But that's for another discussion as you rightfully point.
I did try that change in SIM and this sends `SIMTest` into the most amazing failure scenarios with indexes blocking when they shouldn't, jstaks on all threads blocked and all kind of funnies. I did spend some time on it without success until I realized it should be:
`return isInitialBuild ? LoadType.WRITE : LoadType.READ;`
Correct?
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r426566283
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
> Is it reasonable a 'not ready' index should be serving reads or writes?
For initial build it should't serve reads, but for rebuilds it's more complicated because the index could still be in good condition for most reads. IMO if we were creating the index API from scratch or adding a new implementation probably the best option would be to set not properly (re)built indexes as unable to serve reads and writes. But, given that we have had the cf-based implementation around for a while, I think we should by now preserve their original behaviour and provide the API mechanisms to change that in new implementations, and perhaps also in the standard indexes in the future. That and the recovery task seem a pair of nice improvements to ship with this ticket.
Accepting reads and writes when the rebuild has failed makes some sense in the particular case of cf-based regular indexes because they don't have an initialization task and they are based on idempotent operations on the underlaying column family. I'd say they focus in availability over consistency because even failed rebuilds always leave the index is same or better condition than before. In contrast, it's easy to imagine other implementations where a failed rebuild leaves the index completely corrupt and unusable. I think there could easily be use cases out there relying on this behaviour, and I'm afraid of arbitrarily changing that behaviour without coming back. If we still want to move to the new approach, either in this ticket or in a dedicated one, we should probably open a discussion on the mail list to see if there are deployments relaying on the classic behaviour.
Another tempting option to ease the migration of cf-based indexes to the new consistency-over-availability approach would be making the enum returned by `getSupportedLoadTypeOnFailure` configurable through index options, for example:
```
CREATE INDEX my_idx ON t WITH OPTIONS = {'supported_load_on_failed_init': 'WRITE', 'supported_load_on_failed_rebuild': 'ALL'}
```
This is something that perhaps we could do in the followup ticket to change the default behaviour of regular cf-based indexes, while keeping this ticket focused on the API changes and the recovery task.
WDYT? Maybe I'm wrong and there isn't anyone out there relying on the old 2i behaviour and we should go straight to the the new failed-rebuild-means-broken behaviour.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng closed pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng closed pull request #570:
URL: https://github.com/apache/cassandra/pull/570
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425137333
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
@adelapena please notice I changed sthg from your patch here. Specifically the queryable removal upon failure as it would fail [this](https://github.com/apache/cassandra-dtest/pull/69/files#diff-fb360a29e46831ca27d4bb8e77059503L359) which looks legit.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r421303733
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -485,14 +471,15 @@ private void buildIndexesBlocking(Collection<SSTableReader> sstables, Set<Index>
try
{
- logger.info("Submitting index build of {} for data in {}",
+ logger.info("Submitting index recovery/build of {} for data in {}",
indexes.stream().map(i -> i.getIndexMetadata().name).collect(Collectors.joining(",")),
sstables.stream().map(SSTableReader::toString).collect(Collectors.joining(",")));
// Group all building tasks
Map<Index.IndexBuildingSupport, Set<Index>> byType = new HashMap<>();
for (Index index : indexes)
{
+ boolean isRecovery = !index.supportsLoad(Loads.ALL);
Review comment:
This is the only contentious point imo. I don't know if we should call a `Index#isRecoveryNeeded()` method instead rather than trying to infer it at the SIM level. The former assumes `ALL` is mandatory for an index to the 'right', the latter allows for the index implementation to decide that. Both make sense... It's our choice tbh and switching from one to the other in the future it's easy enough anyway.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r426566283
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
> Is it reasonable a 'not ready' index should be serving reads or writes?
For initial build it should't serve reads, but for rebuilds it's more complicated because the index could still be in good condition. IMO if we were creating the index API from scratch or adding a new implementation probably the best option would be to set not properly (re)built index as unable to serve reads and writes. But, given that we have had the cf-based implementation around for a while, I think we should by now preserve their original behaviour and provide the API mechanisms to change that in new implementations and perhaps in the standard indexes in the future. That and the recovery task seems a pair of nice improvements to ship with this ticket.
Accepting reads and writes when failed makes some sense in the particular case of cf-based regular indexes because they don't have an initialization task and they are based on idempotent operations on the underlaying column family. I'd say they focus in availability over consistency because even failed rebuilds always leave the index is same or better condition than before. In contrast, it's easy to imagine other implementations where a failed rebuild leaves the index completely corrupt and unusable. I think there could easily be use cases out there relying on this behaviour, and I'm afraid of arbitrarily changing that behaviour without coming back. If we still want to move to the new approach, either in this ticket or in a dedicated one, we should probably open a discussion in the mail list to see if there are deployments relaying on the classic behaviour.
Another tempting option to ease the migration of cf-based indexes to the new consistency-over-availability approach would be making the enum returned by `getSupportedLoadTypeOnFailure` configurable through index options, for example:
```
CREATE INDEX my_idx ON t WITH OPTIONS = {'supported_load_on_failed_init': 'WRITE', 'supported_load_on_failed_rebuild': 'ALL'}
```
This is something that perhaps we could do in the ticket to change the default behaviour of regular cf-based indexes, while keeping this ticket focused on the API changes and the recovery task.
WDYT? Maybe I'm wrong and there isn't anyone out there relying on the old 2i behaviour and we should go straight to the the new failed-rebuild-means-broken behaviour.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425925073
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Perhaps the problem was assigning `LoadType.NOOP` as the default behaviour. Indeed, removing that line leaves `LoadType#supportsReads()` without usages. I think that, to preserve the behaviour of the default indexes, and to have more flexibility, we should keep that line and make a distinction between failures in the initial build task and failures in rebuilds. As I mention on the dtest review, the classic behaviour would be:
```java
/**
* Returns the type of operations supported by the index in case its building has failed and it's needing recovery.
*
* @param isInitialBuild {@code true} if the failure is for the initial build task on index creation, {@code false}
* if the failure is for a rebuild or recovery.
*/
default LoadType getSupportedLoadTypeOnFailure(boolean isInitialBuild)
{
return isInitialBuild ? LoadType.WRITE : LoadType.ALL;
}
```
I think that each behaviour has advantages and disadvantages depending on the use case and, while I'm quite happy with the changes in the index API, I'm a bit afraid of changing the behaviour of the standard index implementation. 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.
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635762103
Lgtm. 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.
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425137333
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
@adelapena please notice I changed sthg from your path here. Specifically the queryable removal upon failure as it would fail [this](https://github.com/apache/cassandra-dtest/pull/69/files#diff-fb360a29e46831ca27d4bb8e77059503L359) which looks legit.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-633435042
Great, so you can't access them. I can't tell why as they seem to work for me and your ci-cassandra run failed on trying to pull my dtests branch. The joys of CI :-) What error do you get on my links or do you want to rerun yours?
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r421998472
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1375,9 +1427,10 @@ public void testIndexOnFrozenUDT() throws Throwable
execute("INSERT INTO %s (k, v) VALUES (?, ?)", 0, udt1);
String indexName = createIndex("CREATE INDEX ON %s (v)");
+ assertTrue(waitForIndex(keyspace(), tableName, indexName));
Review comment:
I think this is a feature and the whole point of the PR. Not getting writes until the index is ready. But having said that we don't have any commit log now that we changed to ignoring writes instead of failing them. So the only option left is what you say: we forward the writes to the index and he'll have to sort it out... Still it irks me and this behavior is not symmetrical to the read path :shrug:
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena edited a comment on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena edited a comment on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635927896
Committed to master as [595dc61290a9fda15b6765711141039ec9609bb3](https://github.com/apache/cassandra/commit/595dc61290a9fda15b6765711141039ec9609bb3).
We can close this PR and the one for dtest.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635442414
Cassandra CI re-running [utests](https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-test/112/) and [dtests](https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-dtest/134/).
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-634822310
@bereng It seems that `SIMT#initializingIndexNotQueryableButMaybeNotWritableAfterPartialRebuild` and `indexWithFailedInitializationDoesNotChangeQueryabilityNorWritabilityAfterPartialRebuild` are definitively flaky. I haven't dug into it, but it seems that somehow the SIM calls `logAndMarkIndexesFailed` with an unexpected null exception. The failure can be reproduced just running any of the problematic tests multiple times in IntelliJ, or you can see them in the multiplexer runs 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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r420834322
##########
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:
I think it's a personal style thing. Rather than having 2 booleans and having to reason about them, the Enum makes it more 'in your face' imo: `ALL` is clear what it means vs 2 booleans etc. :shrug:
Also maybe I have too much imagination, but I can think of 'DELETE' as a load. You could have 'append-only' indexes in the future that don't support deletes. Maybe premature optimization is being my enemy here... Or sthg with statics for custom indexes...
I prefer you decide what is best here :-). I'll rename to singulars meanwhile in case we kept that option.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r422916204
##########
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(LoadType load)
+ {
+ switch (load)
+ {
+ case ALL:
+ return supportedLoads == LoadType.ALL;
+ case READ:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.READ;
+ case WRITE:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.WRITE;
+ default:
+ return false;
+ }
Review comment:
Fixed to my discretion. Feel free to reopen if I got it wrong.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-634507197
@adelapena apologies for the late reply. I was going down a rabbit hole debugging a failing test yesterday and I didn't get around to this. Rebased and squashed. We'll have to wait for the CI runs to complete now:
- [CI j8](https://app.circleci.com/pipelines/github/bereng/cassandra/35/workflows/54e22147-7fb8-4171-a0be-1f189b9f657d)
- [CI j11](https://app.circleci.com/pipelines/github/bereng/cassandra/35/workflows/4640daf9-6499-499a-9dab-4eb15fac85db)
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r421413678
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -136,6 +135,7 @@
*/
public interface Index
{
+ public static enum LoadType {READ, WRITE, ALL, NONE};
Review comment:
+1 to the new enum names. It would be nice to add a very brief comment about what it is, for the sake of the unaware reader. The static and the semicolon are not needed:
```suggestion
public enum LoadType {READ, WRITE, ALL, NONE}
```
##########
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(LoadType load)
+ {
+ switch (load)
+ {
+ case ALL:
+ return supportedLoads == LoadType.ALL;
+ case READ:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.READ;
+ case WRITE:
+ return supportedLoads == LoadType.ALL || supportedLoads == LoadType.WRITE;
+ default:
+ return false;
+ }
Review comment:
This block of code is repeated in a couple of test index implementations, and it's to expect that other implementations would need to repeat it. I think that defining that `ALL` includes `READ` and `WRITE`, etc. seems more a characteristic of `LoadType` than the index itself, so perhaps we could move this logic to the enum itself, for example:
```java
public enum LoadType {
READ, WRITE, ALL, NONE;
public boolean accepts(LoadType load)
{
switch (this)
{
case ALL:
return true;
case READ:
return load == LoadType.READ || load == LoadType.NONE;
case WRITE:
return load == LoadType.WRITE || load == LoadType.NONE;
default:
return false;
}
}
}
```
That way we would simplify usage later:
```java
@Override
public boolean supportsLoad(LoadType load)
{
return supportedLoadType.accepts(load);
}
```
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -338,20 +355,20 @@ public void markAllIndexesRemoved()
public void rebuildIndexesBlocking(Set<String> indexNames)
{
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);
- }
+ 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);
+ }
Review comment:
Nit: this seems to have missed alignment with an extra tab
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -619,8 +642,16 @@ private synchronized void markIndexesBuilding(Set<Index> indexes, boolean isFull
private synchronized void markIndexBuilt(Index index, boolean isFullRebuild)
{
String indexName = index.getIndexMetadata().name;
- if (isFullRebuild)
+ if (isFullRebuild && index.supportsLoad(Index.LoadType.READ))
+ {
queryableIndexes.add(indexName);
+ logger.info("Index [" + indexName + "] became queryable.");
+ }
+ if (isFullRebuild && index.supportsLoad(Index.LoadType.WRITE))
+ {
+ writableIndexes.add(indexName);
+ logger.info("Index [" + indexName + "] became writable.");
Review comment:
I think that `became` suggests a change of status, as if it weren't writable before. We could either say that is writable without telling if it was so before or, even better, use the boolean returned by `queryableIndexes.add`:
```suggestion
if (writableIndexes.add(indexName))
logger.info("Index [" + indexName + "] became writable.");
```
The same applies to queryable indexes above.
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -424,10 +441,13 @@ public static String getIndexName(String cfName)
assert isIndexColumnFamily(cfName);
return StringUtils.substringAfter(cfName, Directories.SECONDARY_INDEX_NAME_SEPARATOR);
}
-
+
/**
- * 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.
*
+ * If the index doesn't support ALL {@link Index.LoadType} it performs a recovery {@link getRecoveryTaskSupport()}
+ * instead of a build {@link getBuildTaskSupport()}
Review comment:
```suggestion
* If the index doesn't support ALL {@link Index.LoadType} it performs a recovery {@link Index#getRecoveryTaskSupport()}
* instead of a build {@link Index#getBuildTaskSupport()}
```
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -1356,7 +1396,7 @@ public void commit()
for (Index index : indexes)
{
Index.Indexer indexer = index.indexerFor(key, columns, nowInSec, ctx, Type.COMPACTION);
- if (indexer == null)
+ if (indexer == null || !indexManager.isIndexWritable(index))
Review comment:
See comment above about either calling `isIndexWritable` before creating the indexer, or directly passing `writableIndexes` to the `IndexGCTransaction ` constructor, so we don't need to this check.
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -1126,17 +1158,21 @@ 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<>();
+ for (Index i : indexes.values())
+ {
+ Index.Indexer idxr = i.indexerFor(update.partitionKey(), update.columns(), nowInSec, ctx, IndexTransaction.Type.UPDATE);
+ if (idxr != null && isIndexWritable(i))
Review comment:
We could check if the index is writable before creating the indexer:
```suggestion
if (!isIndexWritable(i))
continue;
Index.Indexer idxr = i.indexerFor(update.partitionKey(), update.columns(), nowInSec, ctx, IndexTransaction.Type.UPDATE);
if (idxr != null)
```
The same applies to the other usages of `isIndexWritable` in `IndexGCTransaction#commit()` and `CleanupGCTransaction#commit()`. Alternatively, we could transform the set `SIM#writableIndexes` into a `Map<String, Index>`, and iterate its values this instead of `SIM#indexes` values in the three places. That way we wouldn't even need the `isIndexWritable` method.
##########
File path: src/java/org/apache/cassandra/index/internal/CassandraIndex.java
##########
@@ -79,6 +78,7 @@
protected ColumnFamilyStore indexCfs;
protected ColumnMetadata indexedColumn;
protected CassandraIndexFunctions functions;
+ protected LoadType supportedLoads = LoadType.ALL;
Review comment:
Nit:
```suggestion
protected LoadType supportedLoadType = LoadType.ALL;
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1052,6 +1052,58 @@ public void testIndexQueriesWithIndexNotReady() throws Throwable
execute("DROP index " + KEYSPACE + ".testIndex");
}
}
+
+ @Test
+ public void testIndexWritesWithIndexNotReady() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, ck int, value int, PRIMARY KEY (pk, ck))");
+ String indexName = createIndex("CREATE CUSTOM INDEX ON %s (value) USING '" + BlockingStubIndex.class.getName() + "'");
+
+ execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 1);
+ BlockingStubIndex index = (BlockingStubIndex) getCurrentColumnFamilyStore().indexManager.getIndexByName(indexName);
+ assertEquals(0, index.rowsInserted.size());
+ execute("DROP index " + KEYSPACE + "." + indexName);
+ }
+
+ @Test // A Bad init could leave an index only accepting reads
+ public void testReadOnlyIndex() throws Throwable
+ {
+ String tableName = createTable("CREATE TABLE %s (pk int, ck int, value int, PRIMARY KEY (pk, ck))");
+ String indexName = createIndex("CREATE CUSTOM INDEX ON %s (value) USING '" + ReadOnlyIndex.class.getName() + "'");
+ assertTrue(waitForIndex(keyspace(), tableName, indexName));
+
+ execute("SELECT value FROM %s WHERE value = 1");
+ execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 1);
+
+ ReadOnlyIndex index = (ReadOnlyIndex) getCurrentColumnFamilyStore().indexManager.getIndexByName(indexName);
+ assertEquals(0, index.rowsInserted.size());
+ execute("DROP index " + KEYSPACE + "." + indexName);
+ }
+
+ @Test // A Bad init could leave an index only accepting writes
+ public void testWriteOnlyIndex() throws Throwable
+ {
+ String tableName = createTable("CREATE TABLE %s (pk int, ck int, value int, PRIMARY KEY (pk, ck))");
+ String indexName = createIndex("CREATE CUSTOM INDEX ON %s (value) USING '" + WriteOnlyIndex.class.getName() + "'");
+ assertTrue(waitForIndex(keyspace(), tableName, indexName));
+
+ execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 1);
+ WriteOnlyIndex index = (WriteOnlyIndex) getCurrentColumnFamilyStore().indexManager.getIndexByName(indexName);
+ assertEquals(1, index.rowsInserted.size());
+ try
+ {
+ execute("SELECT value FROM %s WHERE value = 1");
+ fail();
+ }
+ catch (IndexNotAvailableException e)
+ {
+ assertTrue(true);
+ }
+ finally
+ {
+ execute("DROP index " + KEYSPACE + "." + indexName);
Review comment:
Similarly to the comment before, we could try a recovery, and simulate the initializitation failure throwing an exception in `WriteOnlyIndex#getInitializationTask`.
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1052,6 +1052,58 @@ public void testIndexQueriesWithIndexNotReady() throws Throwable
execute("DROP index " + KEYSPACE + ".testIndex");
}
}
+
+ @Test
+ public void testIndexWritesWithIndexNotReady() throws Throwable
+ {
+ createTable("CREATE TABLE %s (pk int, ck int, value int, PRIMARY KEY (pk, ck))");
+ String indexName = createIndex("CREATE CUSTOM INDEX ON %s (value) USING '" + BlockingStubIndex.class.getName() + "'");
+
+ execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 1);
+ BlockingStubIndex index = (BlockingStubIndex) getCurrentColumnFamilyStore().indexManager.getIndexByName(indexName);
+ assertEquals(0, index.rowsInserted.size());
+ execute("DROP index " + KEYSPACE + "." + indexName);
+ }
+
+ @Test // A Bad init could leave an index only accepting reads
+ public void testReadOnlyIndex() throws Throwable
+ {
+ String tableName = createTable("CREATE TABLE %s (pk int, ck int, value int, PRIMARY KEY (pk, ck))");
+ String indexName = createIndex("CREATE CUSTOM INDEX ON %s (value) USING '" + ReadOnlyIndex.class.getName() + "'");
+ assertTrue(waitForIndex(keyspace(), tableName, indexName));
+
+ execute("SELECT value FROM %s WHERE value = 1");
+ execute("INSERT INTO %s (pk, ck, value) VALUES (?, ?, ?)", 1, 1, 1);
+
+ ReadOnlyIndex index = (ReadOnlyIndex) getCurrentColumnFamilyStore().indexManager.getIndexByName(indexName);
+ assertEquals(0, index.rowsInserted.size());
+ execute("DROP index " + KEYSPACE + "." + indexName);
Review comment:
We could try a recovery here, and verify that the index starts to accepts writes after that recovery. Also, in `ReadOnlyIndex`, we could throw an exception in `getInitializationTask`, and provide a mock implementation `getRecoveryTaskSupport` that enables writes. Note that forcing an exception during initialization would make us to miss the call to `markIndexBuilt`, so the index won't be added to `SIM#queryableIndexes` and it won't accepts reads independently of its `ReadOnlyIndex#supportsLoad` implementation. That's why I think we need some new logic in `SIM#markIndexFailed` too.
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1375,9 +1427,10 @@ public void testIndexOnFrozenUDT() throws Throwable
execute("INSERT INTO %s (k, v) VALUES (?, ?)", 0, udt1);
String indexName = createIndex("CREATE INDEX ON %s (v)");
+ assertTrue(waitForIndex(keyspace(), tableName, indexName));
Review comment:
I think that this is an important change of behaviour in indexes, they are missing writes done during their initialization. I think that this problem can be solved if we add them to `SIM#writableIndexes` at the beginning of their registration (before initialization), and we remove them from `writableIndexes` in `SIM#markIndexFailed`. WDYT?
##########
File path: test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java
##########
@@ -119,37 +124,47 @@ public void addingSSTablesMarksTheIndexAsBuilt() throws Throwable
}
@Test
- public void cannotRebuildWhileInitializationIsInProgress() throws Throwable
+ public void cannotRebuilRecoverdWhileInitializationIsInProgress() throws Throwable
Review comment:
```suggestion
public void cannotRebuildRecoverWhileInitializationIsInProgress() throws Throwable
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1560,4 +1613,77 @@ public IndexBlockingOnInitialization(ColumnFamilyStore baseCfs, IndexMetadata in
return super.getInvalidateTask();
}
}
+
+ /**
+ * <code>StubIndex</code> that blocks during the initialization.
+ */
+ public static class BlockingStubIndex extends StubIndex
+ {
+ private final CountDownLatch latch = new CountDownLatch(1);
+
+ public BlockingStubIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return () -> {
+ latch.await();
+ return null;
+ };
+ }
+
+ @Override
+ public Callable<?> getInvalidateTask()
+ {
+ latch.countDown();
+ return super.getInvalidateTask();
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
+ */
+ public static class ReadOnlyIndex extends StubIndex
+ {
+ public ReadOnlyIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return null;
+ }
+
+ public boolean supportsLoad(LoadType load)
+ {
+ return load.equals(LoadType.READ);
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports writes. Could be intentional or a result of a bad init
Review comment:
Nit: is not a `CassandraIndex`:
```suggestion
* {@code StubIndex} that only supports writes. Could be intentional or a result of a bad init.
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1560,4 +1613,77 @@ public IndexBlockingOnInitialization(ColumnFamilyStore baseCfs, IndexMetadata in
return super.getInvalidateTask();
}
}
+
+ /**
+ * <code>StubIndex</code> that blocks during the initialization.
+ */
+ public static class BlockingStubIndex extends StubIndex
+ {
+ private final CountDownLatch latch = new CountDownLatch(1);
+
+ public BlockingStubIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return () -> {
+ latch.await();
+ return null;
+ };
+ }
+
+ @Override
+ public Callable<?> getInvalidateTask()
+ {
+ latch.countDown();
+ return super.getInvalidateTask();
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
+ */
+ public static class ReadOnlyIndex extends StubIndex
+ {
+ public ReadOnlyIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return null;
+ }
+
+ public boolean supportsLoad(LoadType load)
+ {
+ return load.equals(LoadType.READ);
Review comment:
```suggestion
return load == LoadType.READ;
```
##########
File path: test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java
##########
@@ -719,4 +746,20 @@ public boolean shouldBuildBlocking()
return true;
}
}
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
Review comment:
```suggestion
* <code>TestingIndex</code> that only supports reads. Could be intentional or a result of a bad init
```
##########
File path: test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java
##########
@@ -719,4 +746,20 @@ public boolean shouldBuildBlocking()
return true;
}
}
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
+ */
+ public static class ReadOnlyIndex extends TestingIndex
+ {
+ public ReadOnlyIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ public boolean supportsLoad(LoadType load)
+ {
+ return load.equals(LoadType.READ);
Review comment:
```suggestion
return load == LoadType.READ;
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1560,4 +1613,77 @@ public IndexBlockingOnInitialization(ColumnFamilyStore baseCfs, IndexMetadata in
return super.getInvalidateTask();
}
}
+
+ /**
+ * <code>StubIndex</code> that blocks during the initialization.
+ */
+ public static class BlockingStubIndex extends StubIndex
+ {
+ private final CountDownLatch latch = new CountDownLatch(1);
+
+ public BlockingStubIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return () -> {
+ latch.await();
+ return null;
+ };
+ }
+
+ @Override
+ public Callable<?> getInvalidateTask()
+ {
+ latch.countDown();
+ return super.getInvalidateTask();
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
+ */
+ public static class ReadOnlyIndex extends StubIndex
+ {
+ public ReadOnlyIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return null;
+ }
+
+ public boolean supportsLoad(LoadType load)
+ {
+ return load.equals(LoadType.READ);
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports writes. Could be intentional or a result of a bad init
+ */
+ public static class WriteOnlyIndex extends StubIndex
+ {
+ public WriteOnlyIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return null;
+ }
+
+ public boolean supportsLoad(LoadType load)
+ {
+ return load.equals(LoadType.WRITE);
Review comment:
```suggestion
return load == LoadType.WRITE;
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java
##########
@@ -1560,4 +1613,77 @@ public IndexBlockingOnInitialization(ColumnFamilyStore baseCfs, IndexMetadata in
return super.getInvalidateTask();
}
}
+
+ /**
+ * <code>StubIndex</code> that blocks during the initialization.
+ */
+ public static class BlockingStubIndex extends StubIndex
+ {
+ private final CountDownLatch latch = new CountDownLatch(1);
+
+ public BlockingStubIndex(ColumnFamilyStore baseCfs, IndexMetadata indexDef)
+ {
+ super(baseCfs, indexDef);
+ }
+
+ @Override
+ public Callable<?> getInitializationTask()
+ {
+ return () -> {
+ latch.await();
+ return null;
+ };
+ }
+
+ @Override
+ public Callable<?> getInvalidateTask()
+ {
+ latch.countDown();
+ return super.getInvalidateTask();
+ }
+ }
+
+ /**
+ * <code>CassandraIndex</code> that only supports reads. Could be intentional or a result of a bad init
Review comment:
Nit: it's not a `CassandraIndex `:
```suggestion
* {@code StubIndex} that only supports reads. Could be intentional or a result of a bad init.
```
##########
File path: test/unit/org/apache/cassandra/index/internal/CustomCassandraIndex.java
##########
@@ -77,6 +77,7 @@
protected ColumnFamilyStore indexCfs;
protected ColumnMetadata indexedColumn;
protected CassandraIndexFunctions functions;
+ protected LoadType supportedLoads = LoadType.ALL;
Review comment:
```suggestion
protected LoadType supportedLoadType = LoadType.ALL;
```
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-633601559
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635379425
@adelapena circle CI is on a partial outage and it fails on dtests env set up etc. If you fancy firing one in cassandra ci good, otherwise I'll try to get CI runs again tomorrow #justfyi
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-628653408
@adelapena I have pushed the latest + you can find the dtests PR on the jira ticket. The 2i dtest passes locally also. If you like what you see I'll run the full dtest suite. So far utests look good:
- [CI j11](https://app.circleci.com/pipelines/github/bereng/cassandra/25/workflows/fa67e701-2df7-42a4-9a8e-1edee5200a6c)
- [CI j8](https://app.circleci.com/pipelines/github/bereng/cassandra/25/workflows/429f4fea-de2e-434a-b5a2-99f13b5013ae)
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-634689660
@adelapena The CI runs are complete. The failures I see I think are just flaky tests because I ran some locally and they passed.
The scare comes from the j8/j11UnitTests that fail on `SIMT` (yikes). So I ran that test locally compiled in j8 under j8 and it passes. It passes also compiled on j8 and ran under j11. I checked the logs for the test and it's full of socket and 'too many open files' exceptions. I did run `ant test` under j11 locally and all passed.
Also I managed to rerun that j8 run [here](https://circleci.com/workflow-run/e1f63b50-0393-46ac-a1c4-c529a4ad9907) and you can see now a j8 utest failed but the j11 utests all passed :shrug: (pick your poison lol)
I think this is all CI headaches/noise. It would be great if you could run j8/j11 utests yourself on cassandra ci before merging to be on the safe side. 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.
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425925073
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Perhaps the problem was assigning `LoadType.NOOP` as the default behaviour. Indeed, removing that line leaves `LoadType#supportsReads()` without usages. I think that, to preserve the behaviour of the default indexes, and to have more flexibility, we should keep that line and make a distinction between failures in the initial build task and failures in rebuilds. As I mention on the dtest review, the classic behaviour would be:
```java
/**
* Returns the type of operations supported by the index in case its building has failed and it's needing recovery.
*
* @param isInitialBuild {@code true} if the failure is for the initial build task on index creation, {@code false}
* if the failure is for a rebuild or recovery.
*/
default LoadType getSupportedLoadTypeOnFailure(boolean isInitialBuild)
{
return isInitialBuild ? LoadType.WRITE : LoadType.ALL;
}
```
I think that all the approaches have advantages and disadvantages depending on the use case and, while I'm quite happy with the changes in the index API, I'm a bit afraid of changing the behaviour of the standard index implementation. 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.
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r420827716
##########
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:
I was trying to get all this path without iterators which would be ideal, but `indexes.values()` is a `Collection` which I can't access via `get()`. The previous streams would have the same array conversion, which is suspicious as the immediate solution would be to use the `ArrayList`. Being the hot path maybe we want to keep the 'pure' array index access even though `ArrayLists`'s overhead might be minimal if anyting at all. Sounds good?
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r416641185
##########
File path: src/java/org/apache/cassandra/index/internal/CassandraIndex.java
##########
@@ -689,33 +705,42 @@ private boolean isPrimaryKeyIndex()
@SuppressWarnings("resource")
private void buildBlocking()
{
- baseCfs.forceBlockingFlush();
-
- try (ColumnFamilyStore.RefViewFragment viewFragment = baseCfs.selectAndReference(View.selectFunction(SSTableSet.CANONICAL));
- Refs<SSTableReader> sstables = viewFragment.refs)
+ try
Review comment:
All these changes are just a try-catch sandwiching to change `supportedLoads` to NONE on exceptions.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r425925073
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
I'm not sure this is inline with the `LoadType.NOOP` behaviour of the indexes. Indeed, it leaves `LoadType#supportsReads()` without usages. I think that, to preserve the behaviour of the default indexes, we should make a distinction between failures in the initial build task and failures in rebuilds. As I mention on the dtest review, the current behaviour would be:
```java
/**
* Returns the type of operations supported by the index in case its building has failed and it's needing recovery.
*
* @param isInitialBuild {@code true} if the failure is for the initial build task on index creation, {@code false}
* if the failure is for a rebuild or recovery.
*/
default LoadType getSupportedLoadTypeOnFailure(boolean isInitialBuild)
{
return isInitialBuild ? LoadType.WRITE : LoadType.ALL;
}
```
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-626629871
- [CI j11](https://app.circleci.com/pipelines/github/bereng/cassandra/18/workflows/dcb313cc-b897-4a19-8840-09550a62d6d2)
- [CI j8](https://app.circleci.com/pipelines/github/bereng/cassandra/18/workflows/c3f34b71-5319-4a4b-b6b0-24d90462c71a)
@adelapena 2 things:
- I modified the enum logic
- I didn't quite get the `supportedLoadWhenFailed` suggestion, neither I suspect would bring much value? but I am probably missing sthg. Please feel free to push or rephrase if you feel strongly about it.
Also, reminder to myself to rebase, squahs and run a full dtest suite once we're happy.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-635914587
Looks good to me too, merging.
----------------------------------------------------------------
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
[GitHub] [cassandra] bereng commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r416639964
##########
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<>();
+ for (Index i : indexes.values())
Review comment:
Removing streams from the hot path.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-632630312
@bereng I can't see the links to CI. I'm running [ci-cassandra](https://ci-cassandra.apache.org/job/Cassandra-devbranch/133/) too. Anyway, it seems that we are mostly ready to commit.
> I think what you do here makes perfect sense from the pov you have. Basically you try to infer as much as possible and rely on reasonable assumptions. I still think, call me stubborn :-) , I'd like my approach better. But as you mention this patch brings goodness and if in the future we need to move closer to my pov it's an easy change. So I am +1 on merging this. Having catched the missing flush, the extra testing around things, etc it's all good value.
Now that we have the general mechanism to define queryability and writability on failure, we can open a followup ticket to more aggressively change the default behaviour of Cassandra indexes, possibly with some discussion in the mail list so we don't interfere with existing use cases. If necessary, it wouldn't be difficult to pass the `LoadType` to the CQL index options, so users can decide how their indexes will behave, choosing between availability and consistency. As for failing writes on badly built indexes instead of ignoring them, we should also have some discussion because it will quite aggressively affect the base table/keyspace availability, messing with repairs, compactions, etc. It will also might make sense to have some mechanism to make coordinators aware of which nodes are not available for reads and writes so they can avoid them.
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on pull request #570:
URL: https://github.com/apache/cassandra/pull/570#issuecomment-634742660
> I think this is all CI headaches/noise. It would be great if you could run j8/j11 utests yourself on cassandra ci before merging to be on the safe side. Wdyt?
@bereng Most probably it's just CI noise. I'm running cassandra-ci, now that I'm able to login into it again:
|utest|dtest|
|-----|-----|
|[108](https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-test/108/)|[126](https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-dtest/126/)|
Unfortunately I don't know if there is a way to specify the Java version in cassandra-ci.
One of the errors seems to occur in `SecondaryIndexManagerTest`. We might have made it flaky, so I'm running our classic private multiplexer [here](https://jenkins-cassandra.datastax.lan/view/Parameterized/job/parameterized-testall/650/) and [here](https://jenkins-cassandra.datastax.lan/view/Parameterized/job/parameterized-testall/651/) 🤞
----------------------------------------------------------------
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
[GitHub] [cassandra] adelapena commented on a change in pull request #570: CASSANDRA-13606 Improve handling of 2i initialization failures
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #570:
URL: https://github.com/apache/cassandra/pull/570#discussion_r426791652
##########
File path: src/java/org/apache/cassandra/index/SecondaryIndexManager.java
##########
@@ -655,6 +695,9 @@ private synchronized void markIndexFailed(Index index)
SystemKeyspace.setIndexRemoved(baseCfs.keyspace.getName(), indexName);
needsFullRebuild.add(indexName);
+
+ if (!index.getSupportedLoadTypeOnFailure().supportsWrites() && writableIndexes.remove(indexName) != null)
+ logger.info("Index [" + indexName + "] became not-writable.");
Review comment:
Hi @bereng. I understand that the original behaviour was:
```java
return isInitialBuild ? LoadType.WRITE : LoadType.ALL;
```
That's because the indexes were always writable independently of any kind of build failures, there wasn't even a notion of writability. And, once they were queryable once, they were queryable forever.
Just took a quick look at the `SIMTest` failures, I think they are happening because `TestingIndex.shouldFailCreate` is wrongly missed from `TestingIndex.clear()`. Once it's fixed to reset it, and we change a couple of assertions about writability in `initializingIndexNotQueryableButWritableAfterPartialRebuild`, all the tests seem to pass, at least locally.
I'll take a closer look tomorrow.
----------------------------------------------------------------
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