Skip to content

CASSANDRA-18195 Feature Flag for Accord Transactions #2120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
maedhroz wants to merge 2 commits into from
Closed

CASSANDRA-18195 Feature Flag for Accord Transactions #2120

maedhroz wants to merge 2 commits into from

Conversation

@maedhroz
Copy link
Contributor

@maedhroz maedhroz commented Jan 30, 2023 •
edited
Loading

patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-18195

@maedhroz
Copy link
Contributor Author

maedhroz commented Jan 30, 2023

https://app.circleci.com/pipelines/github/maedhroz/cassandra?branch=CASSANDRA-18195

@maedhroz
Copy link
Contributor Author

maedhroz commented Jan 30, 2023 •
edited
Loading

This patch adds a cassandra.yaml flag that, by default, does the following:

1.) At startup, prevents...
a.) ...Accord virtual tables from being created
b.) ...the Accord system keyspace/tables from being created
c.) ...the AccordService from being initialized, along w/ the threads it owns. (The implementation involving a no-op IAccordService might be up for discussion, but the idea seems to have been agreed to on the Jira already.)
d.) ...operators from enabling Accord-based CAS operations while Accord itself is disabled.

2.) In general, prevents any CQL Accord transaction from executing.

dcapwell
dcapwell reviewed Jan 30, 2023
View reviewed changes
src/java/org/apache/cassandra/service/accord/IAccordService.java Outdated
Copy link
Contributor

@dcapwell dcapwell Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda wish we could just return Node, but since that is what owns the threads... I doubt we can =(

dcapwell
dcapwell requested changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@dcapwell dcapwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small comment about jvm-default, but overall LGTM +1

test/conf/cassandra.yaml Outdated
Copy link
Contributor

@dcapwell dcapwell Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you need to change this config for JVM dtest as well?

Copy link
Contributor Author

@maedhroz maedhroz Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it in AccordTestBase, of course, but outside that, my goal was to keep the rest of the in-JVM test landscape running w/ the default, which right now means accord_transactions_enabled: false. Right trade-off, or am I thinking about it wrong?

Copy link
Contributor Author

@maedhroz maedhroz Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what...I think you're right. It's probably better to have all of this active in a testing context so we ferret out problems earlier. Changing...

@maedhroz
Copy link
Contributor Author

maedhroz commented Jan 31, 2023

Fixed a couple test failures related to the simulator and LOCAL_SYSTEM_KEYSPACE_NAMES...tests running again...

@maedhroz maedhroz force-pushed the CASSANDRA-18195 branch 2 times, most recently from 3dc3e89 to 2511255 Compare January 31, 2023 16:42
dcapwell
dcapwell approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@dcapwell dcapwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@maedhroz
Feature Flag for Accord Transactions
29ec3a1 
patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-18195
@maedhroz
higher CircleCI settings
a70934f
@maedhroz
Copy link
Contributor Author

maedhroz commented Feb 1, 2023

Committed as 2e680a3

@maedhroz maedhroz closed this Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcapwell dcapwell dcapwell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maedhroz @dcapwell