Skip to content
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

Harry 2 #3689

Open
wants to merge 31 commits into
base: cep-15-accord
Choose a base branch
from
Open

Harry 2 #3689

wants to merge 31 commits into from

Conversation

ifesdjeen
Copy link
Contributor

Make it easier to reuse generators, make Harry more extensible and accord-proof, refactor Harry's major subsystems:

  • SELECT is now a regular Harry operation, not a separate "validate" step
  • Flushes (or any custom operation) are now a part of a model, Harry will run them for you
  • Simplified validation of subset/wildcard queries
  • All Operations and are now SchemaSpec-agnostic, and can use any generators (like QT or whatever)
  • You read it right, Harry now can use any generator and still be able to not to keep generated values in memory
  • There are no concepts of “descriptor selectors”, or “bijection” exposed anymore. You just provide a Generator, Harry figures out the rest.
  • All Cassandra data types are now supported
  • No upper limit on the number of columns (also, clustering columns)
  • Basic (for now) support for Transactions
  • It is now possible to visit multiple partition in the same Visit (essentially, for transactions)
  • Much much simpler to generate synthetic workloads now (basically, you just write a Generator
  • Add support for all single-cell types.

Most of it was just a refactoring of Harry, to allow the following (still pending):

  • Easier to add subset SELECTION (important for SAI testing)
  • Easier add support for functions
  • Easier to add support for collections and UDTs
  • Easier to add support for LET bindings

@ifesdjeen
Make it easier to reuse generators, make Harry more extensible and ac…
abfc8d4 
…cord-proof, refactor Harry's major subsystems

  * SELECT is now a regular Harry operation, not a separate "validate" step
  * Flushes (or any custom operation) are now a part of a model, Harry will run them for you
  * Simplified validation of subset/wildcard queries
  * All Operations and are now SchemaSpec-agnostic, and can use any generators (like QT or whatever)
  * You read it right, Harry now can use any generator and still be able to not to keep generated values in memory
  * There are no concepts of “descriptor selectors”, or “bijection” exposed anymore. You just provide a Generator, Harry figures out the rest.
  * All Cassandra data types are now supported
  * No upper limit on the number of columns (also, clustering columns)
  * Basic (for now) support for Transactions
  * It is now possible to visit multiple partition in the same Visit (essentially, for transactions)
  * Much much simpler to generate synthetic workloads now (basically, you just write a Generator
  * Add support for all single-cell types.

Most of it was just a refactoring of Harry, to allow the following (still pending):

  * Easier to add subset SELECTION (important for SAI testing)
  * Easier add support for functions
  * Easier to add support for collections and UDTs
  * Easier to add support for LET bindings
@ifesdjeen
Add transactional mode; disable timestamps when accord is used
abb019b
@ifesdjeen
Improve timestamp handling
7617a54
@ifesdjeen
Address David's comments
a37982f
dcapwell
dcapwell reviewed Nov 19, 2024
View reviewed changes
test/distributed/org/apache/cassandra/distributed/test/accord/AccordHostReplacementTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/log/FuzzTestBase.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/log/MetadataChangeSimulationTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/log/ModelState.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/log/OperationalEquivalenceTest.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/ColumnSpec.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/ColumnSpec.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/ColumnSpec.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/SchemaSpec.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/checker/TestHelper.java Show resolved Hide resolved
dcapwell
dcapwell reviewed Nov 20, 2024
View reviewed changes
src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/test/accord/AccordHostReplacementTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/distributed/upgrade/ClusterMetadataUpgradeHarryTest.java Show resolved Hide resolved
test/distributed/org/apache/cassandra/fuzz/ring/LossyNetworkTest.java Outdated Show resolved Hide resolved
test/distributed/org/apache/cassandra/fuzz/ring/LossyNetworkTest.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/gen/SchemaGenerators.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/gen/SchemaGenerators.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/gen/ValueGenerators.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/gen/rng/PureRng.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/model/QuiescentChecker.java Show resolved Hide resolved
dcapwell
dcapwell reviewed Nov 20, 2024
View reviewed changes
test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/SchemaSpec.java Outdated Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/execution/CQLVisitExecutor.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/execution/QueryBuildingVisitExecutor.java Show resolved Hide resolved
test/harry/main/org/apache/cassandra/harry/gen/ValueGenerators.java Outdated Show resolved Hide resolved
test/unit/org/apache/cassandra/cql3/ast/Select.java Outdated
@@ -60,25 +60,26 @@ public class Select implements Statement
public final Optional<OrderBy> orderBy;
public final Optional<Value> limit;
public final boolean allowFiltering;

public final boolean insertNewLine;
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this from the state, we can add toCQL versions that allow overriding this

patch to fix here: dcapwell@1c8c349

Given these are not harry apis, and APIs I am maintaining, this is a blocking comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to soften what I mean by "blocking comment" here... Alex and I spoke in slack and I stated that most of my comments are suggestions and are non-blocking comments, if we disagree his view wins. I said I would be explicit about what is blocking (as the default isn't) but then this comes off very harsh...

My concerns here is putting the string formatting state into the object rather than providing it to the toCQL directly... I am open to other proposals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed insertNewLine, replaced newlines with space in Harry compile code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want you thinking you can not make improvements to the ast package... I was trying to express formatting decisions should be given to the logic that does the formatting rather than part of the object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maedhroz maedhroz maedhroz left review comments

@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.
3 participants
@ifesdjeen @maedhroz @dcapwell