CASSANDRA-13606 Improve handling of 2i initialization failures

[bereng]

No description provided.

[bereng]

• j11 CI
• j8 CI

[bereng]

• Ci j11
• CI j8

^are these runs good enough or do you recommend I run any other dtests? Thx in advance

[adelapena]

This is looking good. My major concern is that indexes seem to be missing writes done during initialization. This happens because they are not included into SIM#writableIndexes until SIM#markIndexBuilt is called. I think we should add indexes to writableIndexes as soon as they are registered, and remove them in markIndexFailed. Conversely, recovering an index would optimistically mark it as writable as soon as the task starts, setting it as not writable only if it fails, so we don't miss writes done while the recovery task is running.

That way, the meaning of "unavailable for writes" would be that we skip writes as an optimization when we know that we are going to need to do them again during recovery.

Also, the implementation of Index#supportsLoad is based on an internal state that index implementations manage with try/catch blocks in their initialization task. I think that most of that can be done in the SIM, replacing Index#supportsLoad(LoadType) by a simpler Index#supportedLoadWhenFailed getter, returning the LoadType that is supported when the index is failed. That way, SIM#markIndexBuilt would take care of re-adding the index to writable/queryableIndexes in recoveries, and SIM#markIndexFailed would be the only caller of Index#supportedLoadWhenFailed. If we wanted more flexibility in what operations are supported during the initial build, we could always have an Index#supportedLoadWhenCreating.

What do you think? Does it make any sense?

[bereng]

• CI j11
• CI j8

@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, squash and run a full dtest suite once we're happy.

[adelapena]

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.

I'll try to explain myself better. Currently we are managing the writability/queryability status of each index in two separate places:

• In the SIM, with the writableIndexes/queryableIndexes collections.
• In the Index instances, that hold an instance of LoadType, manipulated in the build tasks and exposed through the Index#supportsLoad method.

I think that this duplicate status management can be simplified, so the SIM would be the only responsible for the writability/queryability status management, and the indexes would merely express their capabilities/preferences when the build has failed. To do so:

• For Index, instead of holding a LoadType instance updated with a try-catch during its build, we would just have an Index#supportedLoadTypeWhenNotBuilt() (or similar name) getter method that would return the LoadType that the index supports in case of not being properly build. That method would always return the same value, independently of the sate of the index, because it would only describe a capability of the index implementation.
• SecondaryIndexManager would have all the information regarding index status, so it wouldn't need to query the indexes by their internal status. Instead, it would just ask the indexes: "what kind of loads do you support in case you are not completely built?". This way, the SIM status management would more or less be:
  • On starting index initial build or recovery, always add the index to writableIndexes and perhaps remove it from queryableIndexes if the index LoadType-when-unbuilt policy says so.
  • On markIndexBuilt, always add the index to both writableIndexes and queryableIndexes.
  • On markIndexFailed, remove the index from writableIndexes if its LoadType-when-unbuilt policy says so.

The main benefit of this would be simplifying writability/queryability status management, while minimizing the index implementation responsibilities.

We of course would lose some flexibility in terms of implementations that decide to be always read-only or write-only, or that can accept different loads depending on the type of build failure, etc. But I don't think we currently have a use case for them, and I'd prefer to focus in what we strictly need. In any case, I think it wouldn't be hard for those hypothetical implementations to do all these things without specific SIM support.

What do you think?

[bereng]

@adelapena thanks for taking the time to rephrase that bit about the indexes and the load type.

You're mainly replacing the supported load type variable in Index with a method call and removing that state. Notice CassandraIndex defaults to ALL for the state and the Index#supportsLoad() method defaults to always true except for NONE. I would argue that makes any new index implementation concerns for that a zero overload. Like you can ignore it as defaults are already reflecting the std behavior.

So given the above it boils down to me seeing this as a feature and you probably as an unnecessary optimization. I'd obviously like to leave it as it is (but I am the father of the baby lol) but you know the codebase an it's uses better. So let me know if you'd like me to change it please?

[adelapena]

You're mainly replacing the supported load type variable in Index with a method call and removing that state. Notice CassandraIndex defaults to ALL for the state and the Index#supportsLoad() method defaults to always true except for NONE. I would argue that makes any new index implementation concerns for that a zero overload. Like you can ignore it as defaults are already reflecting the std behavior.

Indeed new index implementations don't need to do anything if the default behaviour is always accepting both reads and writes, independently of whether the index is properly built or not. I'm not sure that's the ideal default behaviour, especially when we are shipping a different behaviour for the standard indexes. Now, if we wanted to change the default behaviour, we would need to manage the their internal state in the index implementations, as we do in CassandraIndex.

In contrast, using a simple getter to retrieve the LoadType would make it easy for implementations to define their behaviour, independently of which default behaviour we have chosen. It will of course come at the cost of losing the feature of allowing indexes to be completely read-only of write-only. As I said, I'm not sure how useful is that feature, and it seems to me that nevertheless it would quite trivial for indexes to implement such behaviour without any specific API support.

I have put here a draft of what the alternative API would look like, in case you want to take a look.

Said that, and although it would not appear to be the case, I'm not super strong on this, and I'll be willing to keep the current method I you think that there might be cases where it can be useful.

I'm running dtests here. Particularly, secondary_indexes_test contains tests about building status and queryability after build failure, node restart, etc. We should check that those still work, and see if we want to extend them in some way.

[bereng]

@adelapena again, thanks a lot for looking into this. Given you've put the time into implementing your approach I have patched and adopted it. At the end of the day you're more familiar with the code and it's uses than me :-)

The dtests thing... mmmm we should try to have as much utest and as little dtest as possible imo. Let me go through them, as they're new to me, and get back to you.

Btw your dtest run looks good. I guess we didn't break much stuff :-)

Edit: On the recent 4.0 status email do you have any strong opinions on this ticket? I don't expect dtests will need much and we're close to merging it seems. I would vote to leave this in 4.0 being so close. My 2cts.

[adelapena]

@adelapena again, thanks a lot for looking into this. Given you've put the time into implementing your approach I have patched and adopted it. At the end of the day you're more familiar with the code and it's uses than me :-)

Great, have you pushed the merge?

The dtests thing... mmmm we should try to have as much utest and as little dtest as possible imo. Let me go through them, as they're new to me, and get back to you

Right, I was thinking of testing the log messages informing about when an index becomes queryable/writable. The dtest test_failing_manual_rebuild_index looks ideal for this purpose, we could easily add node.grep_log checks after each index (re)build to check that it behaves at is supposed. I don't know if we have tools to verify logs and simulate node restarts in utest.

Btw your dtest run looks good. I guess we didn't break much stuff :-)

This is good news, it seems that at least we didn't make it worse than it was 😃

Edit: On the recent 4.0 status email do you have any strong opinions on this ticket? I don't expect dtests will need much and we're close to merging it seems. I would vote to leave this in 4.0 being so close. My 2cts.

I think we are close to merge. Extending dtests should be a quite small task, probably just the few log checks mentioned above. It's true that the ticket doesn't solve any serious problem that we have with current implementations, but it's not very invasive and it's almost done, so I would also be in favour of keeping it in 4.0

[bereng]

@adelapena please notice I changed sthg from your patch here. Specifically the queryable removal upon failure as it would fail this which looks legit.

[adelapena]

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:

/**
 * 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?

[bereng]

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 🤷

[adelapena]

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.

[bereng]

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?

[adelapena]

Hi @bereng. I understand that the original behaviour was:

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.

[bereng]

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.

[adelapena]

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!

[bereng]

@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
• CI j8

[adelapena]

I have left a few suggested changes here. Changes for the dtest are here.

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?

[bereng]

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.

[bereng]

CI runs:

• CI j8
• CI j11

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.

[adelapena]

@bereng I can't see the links to CI. I'm running ci-cassandra 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.

[bereng]

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?

[adelapena]

I see a message telling Workflows >> null >> null >> null >>. However I can see the list of your workflows on https://circleci.com/gh/bereng/workflows/cassandra , but I can't see the detail of any of them 🤷

Currently I'm having trouble to login at ci-cassandra and re-run, I'll try later.

[adelapena]

I have managed to see your CI run. It seems that CircleCI was punishing me for using the old UI, and at the same time it wasn't showing me the option to upgrade to the new UI 🤦

The results look good to me to. SlowQueryLog is failing because dtests include CASSANDRA-15503 but this PR doesn't. Perhaps we should squash+rebase and run a final CI round, and we'll be ready to merge, WDYT?

[bereng]

@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
• CI j11

[bereng]

@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 and you can see now a j8 utest failed but the j11 utests all passed 🤷 (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?

[adelapena]

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	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 and here 🤞

[adelapena]

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

[bereng]

@adelapena ok that took a while to pin down. There were concurrency issues with the sets keeping track of the indices, which lead to wrong failedIndexes lists triggering the NPE. The fix is here. It was a legit bug :-)

I did run the test 1K times and it passed. Now waiting on CI.

Edit: Started CI again upon a GH connection error that failed them...

[adelapena]

Nice detective work. So it seems that by introducing multiple indexes in the test we have found and fixed and unrelated bug in the SIM, that's a good thing.

If CI looks we'll finally be ready to merge

[bereng]

@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

[adelapena]

Cassandra CI re-running utests and dtests.

[bereng]

Lgtm. Wdyt?

[adelapena]

Looks good to me too, merging.

[adelapena]

Committed to master as 595dc61290a9fda15b6765711141039ec9609bb3.
We can close this PR and the one for dtest.