alternator: document the state of tablet support in Alternator#22462
alternator: document the state of tablet support in Alternator#22462nyh wants to merge 1 commit intoscylladb:masterfrom
Conversation
github-actions
bot
added
documentation
Docs Preview 📖Docs Preview for this pull request is available here Changed Files: Note: This preview will be available for 30 days and will be automatically deleted after that period. You can manually trigger a new build by committing changes. |
🔴 CI State: FAILURE✅ - Build Failed Tests (120/27155):
Build Details:
|
docs/alternator/new-apis.md
Outdated
| * Enabling TTL with UpdateTableToLive doesn't work (results in an error). | ||
| See <https://github.com/scylladb/scylla/issues/16567>. | ||
|
|
||
| * Enabling Streams with during CreateTable or UpdateTable doesn't work |
There was a problem hiding this comment.
Thanks. Fixed in the upcoming version.
test/alternator/test_tablets.py
Outdated
| 'KeySchema': [ { 'AttributeName': 'p', 'KeyType': 'HASH' } ], | ||
| 'AttributeDefinitions': [ { 'AttributeName': 'p', 'AttributeType': 'S' }]} | ||
| with new_test_table(dynamodb, **schema) as table: | ||
| # Change this assertion if Scylla's default changes! |
There was a problem hiding this comment.
You mean Scylla's default for Alternator? As the default for regular tables is unrelated if I understand correctly.
There was a problem hiding this comment.
Yes, the comment a few lines up explain this more clearly
# Right now, new Alternator tables are created *without* tablets.
# This test should be changed if this default ever changes.
This one-line comment just meant to point out exactly which line of code will need to change - I didn't want to repeat the full text there. I'll change the word "Scylla" to "Alternator" to make this slightly less mysterious.
I had a silly accident in the patch (moved the wrong line between two files) which caused all these pre-existing (and unchanged in this patch) tests to file. Will send a fixed version. |
|
@nyh new branch |
|
CI hasn't finished in 4 days, I don't know how how to check why. I'll rebase to get a new CI running. |
🔴 CI State: ABORTED✅ - Build Failed Tests (7/303355):
Build Details:
|
Seems like a previously-unknown flakiness, I opened #22745 with the details. It may be the same as #22647 but I just wasn't sure.
Maybe the same or very similar thing as above... This time repair-based streaming dies instead of raft-based streaming. I reported it in the same issue, I hope it will help someone figure out the root cause.
This seems to be a time out on a CREATE KEYSPACE in cassandra stress... , I don't even know what to report here. I can't find any errors in the logs, the server seems to have created the keyspace just fine. Maybe it's another case of a driver retransmission? But I can't find evidence in the logs.
I couldn't figure this out either. To be honest, I don't have the energy to look into all these bugs in tests I'm not familiar with to code I'm not familiar with - or even report all of them :-( |
|
Rebased to get CI running again and hopefully less flakiness. |
|
By the way, it seems that Github has a new bug that @avikivity already noticed a few days ago: The only think I did right now is to rebase and push a new version. I did not request any review. And yet, github is lying above, claiming that I requested a review from @ptrsmrn. So, @ptrsmrn you're very welcome to review this PR, but I did not explicitly requested your review. Github just faked this request supposedly from me :-( |
🔴 CI State: FAILURE✅ - Build Failed Tests (1/95575):Build Details:
|
|
Nit, in the commit msg: "When the tablets feature finally will finally completed" needs to be rephrased. |
| number of initial tablets. But any other number can be used, and this | ||
| number overrides the default choice of initial number of tablets. |
There was a problem hiding this comment.
"But any other number can be used", yes, but AFAIK this number is going to be rounded to the next power of 2.
There was a problem hiding this comment.
I don't want to include here more detailed explanation of what this "initial tablets" number means, especially when I see that the tablets developers keep changing its meaning. They now have per-shard numbers vs. not-per-shard, they have "initial" vs "minimum" numbers, they have per-keyspace defaults as well as per-table defaults. They even have a multiplier (!) that multiplies this number, so you ask for "1" but actually get 10. I lost track. This needs to be documented elsewhere (is it?), but not here.
To be honest, the only really interesting setting for this tag is 0 (use tablets) and "none" (or any other non-number) to disable tablets. Nobody will ever want to use 3 or 7. I think.
In commit c24bc3b we decided that creating a new table in Alternator will by default use vnodes - not tablets - because of all the missing features in our tablets implementation that are important for Alternator, namely - LWT, CDC and Alternator TTL. We never documented this, or the fact that we support a tag `experimental:initial_tablets` which allows to override this decision and create an Alternator table using tablets. We also never documented what exactly doesn't work when Alternator uses tablet. This patch adds the missing documentation in docs/alternator/new-apis.md (which is a good place for describing the `experimental:initial_tablets` tag). The patch also adds a new test file, test_tablets.py, which includes tests for all the statements made in the document regarding how `experimental:initial_tablets` works and what works or doesn't work when tablets are enabled. Two existing tests - for TTL and Streams non-support with tablets - are moved to the new test file. When the tablets feature will finally be completed, both the document and the tests will need to be modified (some of the tests should be outright deleted). But it seems this will not happen for at least several months, and that is too long to wait without accurate documentation. Fixes scylladb#21629 Signed-off-by: Nadav Har'El <[email protected]>
Why? It's pretty accurate. Right now, and in the near future, Alternator tables are created without tablets and you have an option to make them use tablets, but then you lose some things (like ability to use three-letter acronyms like GSI, LWT, TTL or CDC). In this patch I document this state, and the commit message states that "when the tablets feature will finally be completed" (i.e., it supports GSI, LWT, TTL and CDC) we'll need to revisit this document and the test. |
|
Rebased, no other changes. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
|
@scylladb/scylla-maint please consider merging. |
In commit c24bc3b we decided that creating a new table in Alternator will by default use vnodes - not tablets - because of all the missing features in our tablets implementation that are important for Alternator, namely - LWT, CDC and Alternator TTL. We never documented this, or the fact that we support a tag `experimental:initial_tablets` which allows to override this decision and create an Alternator table using tablets. We also never documented what exactly doesn't work when Alternator uses tablet. This patch adds the missing documentation in docs/alternator/new-apis.md (which is a good place for describing the `experimental:initial_tablets` tag). The patch also adds a new test file, test_tablets.py, which includes tests for all the statements made in the document regarding how `experimental:initial_tablets` works and what works or doesn't work when tablets are enabled. Two existing tests - for TTL and Streams non-support with tablets - are moved to the new test file. When the tablets feature will finally be completed, both the document and the tests will need to be modified (some of the tests should be outright deleted). But it seems this will not happen for at least several months, and that is too long to wait without accurate documentation. Fixes scylladb#21629 Signed-off-by: Nadav Har'El <[email protected]> Closes scylladb#22462 (cherry picked from commit c082184)
In commit c24bc3b we decided that creating a new table in Alternator will by default use vnodes - not tablets - because of all the missing features in our tablets implementation that are important for Alternator, namely - LWT, CDC and Alternator TTL. We never documented this, or the fact that we support a tag `experimental:initial_tablets` which allows to override this decision and create an Alternator table using tablets. We also never documented what exactly doesn't work when Alternator uses tablet. This patch adds the missing documentation in docs/alternator/new-apis.md (which is a good place for describing the `experimental:initial_tablets` tag). The patch also adds a new test file, test_tablets.py, which includes tests for all the statements made in the document regarding how `experimental:initial_tablets` works and what works or doesn't work when tablets are enabled. Two existing tests - for TTL and Streams non-support with tablets - are moved to the new test file. When the tablets feature will finally be completed, both the document and the tests will need to be modified (some of the tests should be outright deleted). But it seems this will not happen for at least several months, and that is too long to wait without accurate documentation. Fixes #21629 Signed-off-by: Nadav Har'El <[email protected]> Closes #22462 (cherry picked from commit c082184) Closes #23298
In commit c24bc3b we decided that creating a new table in Alternator will by default use vnodes - not tablets - because of all the missing features in our tablets implementation that are important for Alternator, namely - LWT, CDC and Alternator TTL. We never documented this, or the fact that we support a tag `experimental:initial_tablets` which allows to override this decision and create an Alternator table using tablets. We also never documented what exactly doesn't work when Alternator uses tablet. This patch adds the missing documentation in docs/alternator/new-apis.md (which is a good place for describing the `experimental:initial_tablets` tag). The patch also adds a new test file, test_tablets.py, which includes tests for all the statements made in the document regarding how `experimental:initial_tablets` works and what works or doesn't work when tablets are enabled. Two existing tests - for TTL and Streams non-support with tablets - are moved to the new test file. When the tablets feature will finally be completed, both the document and the tests will need to be modified (some of the tests should be outright deleted). But it seems this will not happen for at least several months, and that is too long to wait without accurate documentation. Fixes scylladb#21629 Signed-off-by: Nadav Har'El <[email protected]> Closes scylladb#22462 (cherry picked from commit c082184) Closes scylladb#23298
5 participants
In commit c24bc3b we decided that creating a new table in Alternator will by default use vnodes - not tablets - because of all the missing features in our tablets implementation that are important for Alternator, namely - LWT, CDC and Alternator TTL.
We never documented this, or the fact that we support a tag
experimental:initial_tabletswhich allows to override this decision an create an Alternator table using tablets. We also never documented what exactly doesn't work in Alternator uses tablet.This patch adds the missing documentation in docs/alternator/new-apis.md (which is a good place for describing the
experimental:initial_tabletstag). The patch also adds a new test file, test_tablets.py, which includes tests for all the statements made in the document regarding howexperimental:initial_tabletsworks and what works or doesn't work when tablets are enabled.Two existing tests - for TTL and Streams non-support with tablets - are moved to the new test file.
When the tablets feature finally will finally completed, both the document and the tests will need to be modified (some of the tests should be outright deleted). But it seems this will not happen for at least several months, and that is too long to wait without accurate documentation.
Fixes #21629