alternator: enable tablets by default if experimental feature is enabled#16900
alternator: enable tablets by default if experimental feature is enabled#16900scylladb-promoter merged 3 commits intoscylladb:masterfrom
Conversation
|
By the way, @bhalevy @tgrabiec @denesb I noticed (but didn't do any formal measurements to "prove" it) that it seems that dropping a keyspace is significantly slower with tablets (with the default number of initial_tablets) than with vnodes. The Alternator test suite creates a dozen or so tables for all tests to use, and then the suite ends with deleting all these tables. This deletion phase seems to take less than a second with vnodes, but almost 10 seconds with tablets. It might be worth measuring whether deleting a table - with snapshots disabled - is significantly slower with tablets than it is without them. Is there a lot of extra raft computations that needs to be done (note that this test only has one node!)? A lot of unnecessary compactions (note that when a table is going to be dropped, we shouldn't spend time compacting it!)? Something else? Do we have any performance benchmarks for drop table with and without tablets? CC @roydahan |
|
I'm guessing there is some amplification where the destruction of a tablet replica updates the table (to clear cache), and that's done once per tablet replica, so amplifies the cost. |
| # temporary usefulness, and should eventually be removed. | ||
| if '--vnodes' in sys.argv: | ||
| sys.argv.remove('--vnodes') | ||
| remove_scylla_options.append('--experimental-features=tablets') |
There was a problem hiding this comment.
Doesn't this mean that we'll default to not testing alterantor streams and ttl any more?
There was a problem hiding this comment.
Yes, it sort of does, because indeed these two things no longer work with tablets.
We have the same issue about no longer testing CDC in cql-pytest (since @denesb 's patch for cql-pytest got in)
There are two "solutions" to this observation, if it needs to be solved:
- Have CI run each tests twice (not every developer needs to do this, it will be very annoying).
- The test code could, instead of "xfailing" a CDC test in tablets mode, use a different tablet-less keyspace for the CDC tests. It's not very hard to do, actually. The downside is that it won't be visible that these tests don't work in tablets - they can be in this state that they only work on vnodes for years, and nobody will notice (developers won't see a lot of xfails or skips when running these tests).
Any opinion on what we should do here?
There was a problem hiding this comment.
Let's revert that patch then.
There was a problem hiding this comment.
I don't see any reason to revert the patch - it doesn't break anything in the code (the worst it can break is not to test something). We can do a followup patch to fix the cql-pytest situation in anyway we want CC @denesb .
I'll see what I can do for this patch (Alternator tests) to make you happier. I can try option 2 - CDC and TTL will be temporarily tested without tablets, just so we keep testing them somehow and not just skip them.
But I think option 1 - running all the tests of all types twice, once for vnodes and once for tablets - from the CI, is the only option that will make you feel 100% safe, ensuring that a year from now, we continue to test (even if just once a week, not on every CI run) that all the tests pass even if vnodes are used.
There was a problem hiding this comment.
It breaks our ability to detect regressions. IMO that's critical.
There was a problem hiding this comment.
I will send a PR to run CDC tests with vnodes, until they are made to work with tablets. After which points, they should be ran with both, because they apparently do care about the replication method.
There was a problem hiding this comment.
A tablets specific fix should add a tablets-specific test.
As long as tablets disable some tests, we can't test only tablets.
There was a problem hiding this comment.
"Temporarily" means nothing without a date.
There was a problem hiding this comment.
- Definitely, a tablets-specific fix should add a tablets-specific test. We've been doing this all along - a lot of these tests are in test/topology_*/tablets, and there's even one in test/cql-pytest. But Botond's CQL patch and my Alternator patch is not about those tests, it's about the almost 2000 other single-node functional tests for CQL and Alternator that 99% of them are completely agnostics to tablets vs vnodes.
- Alternator TTL, for example, can probably be fixed fairly easily to work on both tablets and vnodes. Until then, this PR sets the test to run on vnodes instead of tablets. It is "temporary" until the time we do the Alternator TTL fix. If you want a date for this specific fix, I can commit to one, but I got the impression that Dor didn't want to rush Alternator fixes. I just wanted to get the minimum in now.
To be honest, it's not clear to me what you're asking to do. Botond sent in the past a patch which runs every single cql-pytest twice, once with tablets and once without, and you agreed that it's excessive. Now he sent a patch to skip tests for features not yet available for tablets (e.g., CDC), and you didn't like that either. Now I sent a this PR to (in the context of Alternator) run all the tests with tablets and just the missing features (e.g., CDC) will be run but without tablets - you didn't like it either. So, what would you like to do?
There was a problem hiding this comment.
"Temporarily" means nothing without a date.
I will send a PR re-enabling all disabled tests today.
Part of it is already here: #16802.
🟢 CI State: SUCCESS✅ - Build Build Details:
|
I have not been able to prove my complaint - I ran a test that creates (in CQL) a table with zero or 10 or 100,000 short rows, in three cases: vnodes, default number of tablets (I guess 4) and 128. The time to drop the table was low in all cases and the overhead of tablets was measurable but not impressive (maybe 0.02 seconds, certainly didn't get to full seconds). So I don't know what kind of slowdown I was measuring with the Alternator test run, maybe I was just imagining it. I'll try to continue investigating it later. |
|
Due to popular demand (from @avikivity) I pushed a new version which avoids using xfail_tablets or skip_tablets (I still used in one insignificant place, to avoid a mess), and instead all the Alternator Streams and Alternator TTL tests (features which don't yet support tablets) explicitly create their tables without tablets. This means we can run all the tests with tablets and just those specific tests will run without tablets. |
|
Cover letter needs an update. |
🔴 CI State: FAILURE✅ - Build Failed Tests (2/23802):
Build Details:
|
| response = info.query( | ||
| KeyConditions={'keyspace_name': { | ||
| 'AttributeValueList': ['alternator_'+test_table.name], | ||
| 'ComparisonOperator': 'EQ'}}) |
There was a problem hiding this comment.
I do not fully understand this expression, but note that scylla_keyspaces, can have an entry for a given keyspace, for other reasons too, not just when it has tablets enabled. In particular, using non-local storage (S3) will also add an entry in this table. In the future we will possibly have more such features.
There was a problem hiding this comment.
This is old-style DynamoDB key conditions (I don't know why I used the old style here, but it works).
It basically means look for a partition with keyspace_name equal to this expected keyspace name.
I knew that this table may contain other things too. This is why the code below also did:
if 'initial_tablets' in response['Items'][0]:
return True
return False
I.e., I only return true if there's an initial_tablets in the response.
But now that I think about it again, maybe if the row exists, it will automatically have an initial_tablets but it will be a null string. Maybe I need to check not just that initial_tablets is in the response but also that it's a non-empty string. I now remember you did something like this in your code.
I'll change it.
Tablet metadata is dropped together with the table in the same group0 command, and handling in do_schema_merge() will first drop the table, then reload tablet metadata. The increased cost may come from the fact that there are many compaction groups and something is O(compaction group). Looking at table::stop(), it does parallel close: There is an extra cost of reloading tablet metadata in |
Oops, you're right. Done. |
If an Alternator table uses tablets (we'll turn this on in a following patch), some tests are known to fail because of features not yet supported with tablets, namely: Refs scylladb#16317 - Support Alternator Streams with tablets (CDC) Refs scylladb#16567 - Support Alternator TTL with tablets This patch changes all tests failing on tablets due to one of these two known issues to explicitly ask to disable tablets when creating their test table. This means that at least we continue to test these two features (Streams and TTL) even if they don't yet work with tablets. We'll need to remember to remove this override when tablet support for CDC and Alternator TTL arrives. I left a comment in the right places in the code with the relevant issue numbers, to remind us what to change when we fix those issues. This patch also adds xfail_tablets and skip_tablets fixtures that can be used to xfail or skip tests when running with tablets - but we don't use them yet - and may never use them, but since I already wrote this code it won't hurt having it, just in case. When running without tablets, or against an older Scylla or on DynamoDB, the tests with these marks are run normally. Signed-off-by: Nadav Har'El <[email protected]>
Before this patch, Alternator tables did not use tablets even if this feature was available - tablets had to be manually enabled per table by using a tag. But recently we changed CQL to enable tablets by default on all keyspaces (when the experimental "tablets" option is turned on), so this patch does the same for Alternator tables: 1. When the "tablets" experimental feature is on, new Alternator tables will use tablets instead of vnodes. They will use the default choice of initial_tablets. 2. The same tag that in the past could be used to enable tablets on a specific table, now can be used to disable tablets or change the default initial_tablets for a specific table at creation time. Fixes scylladb#16355 Signed-off-by: Nadav Har'El <[email protected]>
test/cql-pytest/run.py was recently modified to add the "tablets" experimental feature, so test/alternator/run now also runs Scylla by default with tablets enabled. This is the correct default going forward, but in the short term it would be nice to also have an option to easily do a manual test run *without* tablets. So this patch adds a "--vnodes" option to the test/alternator/run script. This option causes "run" to run Scylla without enabling the "tablets" experimental feature. Signed-off-by: Nadav Har'El <[email protected]>
|
Pushed a new version. In this version the xfail_tablets / skip_tablets fixtures are not used (I left them behind, since it wasn't easy to write correctly, but do not use them at all). Rather, the tests that can't work on tablets because of missing CDC and TTL support (those are the only two issues) explicitly ask to run without tablets - even if the rest of the tests work with tablets. @avikivity I believe that's what you wanted. A note to reviewers @denesb @avikivity: The main reason why we need this PR is not the tests, it's making tablets enabled in Alternator tables by default (when the "tablets" experimental feature is enabled), like we already did with CQL. The test changes are just something we need to do so we don't start to get test failures as soon as tablets are enabled by default. Please consider merging this PR even if you don't think our approach to testing is perfect yet - I think it's good enough, and again - testing is not the main focus of this PR, it's enabling tablets in Alternator. |
This is known flaky code (not a specific test), unrelated to this patch. Tracked in #16821. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
| # xfailing these tests, we temporarily coerce the tests below to avoid | ||
| # using default tablets setting, even if it's available. We do this by | ||
| # using the following tags when creating each table below: | ||
| TAGS = [{'Key': 'experimental:initial_tablets', 'Value': 'none'}] |
There was a problem hiding this comment.
Btw, in 49026dc Pavel added auto-calculation of initial tablet count, and the user just specifies that the keyspace should be tablet-aware. Do we want to do the same for alternator?
There was a problem hiding this comment.
Yes, the main goal of this PR wasn't to fix the tests (this was a necessary part) but to make using tablets in Alternator automatic, like we do in CQL:
- After this patch, if the "tablets" experimental feature is on, all Alternator tables will use tablets by default, with the default initial_tablets calculation you have in CQL (I pass initial_tablets=0 to the lower-level code).
- The user is given an experimental way to choose a different initial_tablets or to disable tablets completely.
- There is no API to specify that the table should use tablets, because it's already on by default.
|
What happens when we make tablets non-experimental? Alternator streams will stop working, no? |
Yes, just like CDC. We have the same problem with Alternator TTL, although I'm guessing it can be pretty easily fixed for tablets, easier than CDC (#16567). |
|
@scylladb/scylla-maint please consider merging, or if there is something you want to me to fix, please make the request more explicit - I wasn't sure if there's anything left for me to fix. |
|
I will now queue this. @nyh I think you will need to send a follow-up PR to add the |
I think it's not true that test.py bypasses the run script for Alternator. It's true for cql-pytest, but test/alternator/suite.yaml says that it still uses "Run". I think there was an attempt to change it and make test/alternator also not use the run script (which can be helpful for parallelising the Alternator tests, even though all of them together take less time than one dtest ;-)) - but it never actually got in. When it does get in and somebody copies test/cql-pytest/suite.yaml to test/alternator/suite.yaml, they'll also get the experimental-features parameters. |
6 participants
This series does a similar change to Alternator as was done recently to CQL:
Fixes #16355