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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
