cassandra-pr mailing list archives

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

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


Mime
View raw message