alternator: do not use tablets on new Alternator tables#18157
alternator: do not use tablets on new Alternator tables#18157nyh wants to merge 1 commit intoscylladb:masterfrom
Conversation
A few months ago, in merge d3c1be9, we decided that if Scylla has the experimental "tablets" feature enabled, new Alternator tables should use this feature by default - exactly like this is the default for new CQL tables. Sadly, it was now decided to reverse this decision: We do not yet trust enough LWT on tablets, and since Alternator often (if not always) relies on LWT, we want Alternator tables to continue to use vnodes - not tablets. The fix is trivial - just changing the default. No test needed to change because anyway, all Alternator tests work correctly on Scylla with the tablets experimental feature disabled. I added a new test to enshrine the fact that Alternator does not use tablets. An unfortunate result of this patch will be that Alternator tables created on versions with this patch (e.g., Scylla 6.0) will not use tablets and will continue to not use tablets even if Scylla is upgraded (currently, the use of tablets is decided at table creation time, and there is no way to "upgrade" a vnode-based table to be tablet based). This patch should be reverted as soon as LWT support matures on tablets. Signed-off-by: Nadav Har'El <[email protected]>
|
@bhalevy this is the patch to change Alternator to not use tablets by default, undoing a decision made a couple of months ago. Makes me sad to do this - it's a step backwards - but that's what we decided should be done because of the state of LWT with tablets. Note how I didn't need to change any test at all, they all pass with this change, just like all the tests already worked on Scylla without the tablets experimental feature. I added one silly test to "enshrine" the new situation that tablets are not used by default, just to make sure we don't unintentionally start using them. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
|
@nyh If this change reverts previously introduced change, can't you just revert that commit(s)? (Plus introduce a commit with the "silly" test). I'm talking about |
bhalevy
left a comment
bhalevy
left a comment
There was a problem hiding this comment.
LGTM
@ptrsmrn I think that doing the mechanical revert would bring back outdated comments that will need to be fixed with another patch on top of that to explain why the default is what it is. So it's probably isn't worth it. The reference to the merge commit that previously enabled tablets by default is good for paper trail purposes.
|
Indeed, I thought like @bhalevy that the new version should include relevant comments about the current state, not some comments that were relevant three months ago. Moreover, the patch that added the tablet default also added logic to disable tablets if the tag is set to a non-number (including empty value), and although this logic isn't particularly useful when the default is to disable anyway, I didn't want to lose this code - and didn't want explicitly disabling the tablets to become an error, like it was before the original patch. |
A few months ago, in merge d3c1be9, we decided that if Scylla has the experimental "tablets" feature enabled, new Alternator tables should use this feature by default - exactly like this is the default for new CQL tables. Sadly, it was now decided to reverse this decision: We do not yet trust enough LWT on tablets, and since Alternator often (if not always) relies on LWT, we want Alternator tables to continue to use vnodes - not tablets. The fix is trivial - just changing the default. No test needed to change because anyway, all Alternator tests work correctly on Scylla with the tablets experimental feature disabled. I added a new test to enshrine the fact that Alternator does not use tablets. An unfortunate result of this patch will be that Alternator tables created on versions with this patch (e.g., Scylla 6.0) will not use tablets and will continue to not use tablets even if Scylla is upgraded (currently, the use of tablets is decided at table creation time, and there is no way to "upgrade" a vnode-based table to be tablet based). This patch should be reverted as soon as LWT support matures on tablets. Signed-off-by: Nadav Har'El <[email protected]> Closes scylladb#18157
4 participants
A few months ago, in merge d3c1be9, we decided that if Scylla has the experimental "tablets" feature enabled, new Alternator tables should use this feature by default - exactly like this is the default for new CQL tables.
Sadly, it was now decided to reverse this decision: We do not yet trust enough LWT on tablets, and since Alternator often (if not always) relies on LWT, we want Alternator tables to continue to use vnodes - not tablets.
The fix is trivial - just changing the default. No test needed to change because anyway, all Alternator tests work correctly on Scylla with the tablets experimental feature disabled. I added a new test to enshrine the fact that Alternator does not use tablets.
An unfortunate result of this patch will be that Alternator tables created on versions with this patch (e.g., Scylla 6.0) will not use tablets and will continue to not use tablets even if Scylla is upgraded (currently, the use of tablets is decided at table creation time, and there is no way to "upgrade" a vnode-based table to be tablet based).
This patch should be reverted as soon as LWT support matures on tablets.