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.

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

c-s: create correct tables based on workload #86

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

muzarski
Copy link
Collaborator

fix: #85

List of tables created for each command:

  • read/write -> keyspace1.standard1 table
  • counter_read/counter_write -> keyspace1.counter1 table
  • mixed -> both keyspace1.[standard1/counter1] tables
  • user -> keyspace and table provided in user profile yaml file

@muzarski muzarski self-assigned this Jun 10, 2024
@muzarski muzarski requested review from soyacz and wprzytula June 10, 2024 10:30
@muzarski muzarski force-pushed the create_tables branch 2 times, most recently from cbadf27 to 7759463 Compare June 10, 2024 10:42
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

For substantially better review experience (clear diff between old and new logic), please extract another commit that makes the create_schema function a method (without any changes to its logic) (pure refactor commit), and then proceed with the commit that changes logic only.

@muzarski
Copy link
Collaborator Author

v2: addressed review comment

@roydahan
Copy link
Collaborator

  • mixed -> both keyspace1.[standard1/counter1] tables

@muzarski the defined behavior is incorrect.
Actually mixed should be the same as "read/write" and run on table keyspace1.standard1.
The difference is that mixed should do 50/50 reads/writes ops.

Comment on lines 141 to 145
let (create_standard_table, create_counter_table) = match (&self.mixed, &self.counter) {
(None, None) => (true, false),
(None, Some(_)) => (false, true),
(Some(_), _) => (true, true),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is incorrect definition

Copy link
Collaborator

@roydahan roydahan left a comment

Choose a reason for hiding this comment

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

Need to change mixed behavior

@muzarski
Copy link
Collaborator Author

  • mixed -> both keyspace1.[standard1/counter1] tables

@muzarski the defined behavior is incorrect. Actually mixed should be the same as "read/write" and run on table keyspace1.standard1. The difference is that mixed should do 50/50 reads/writes ops.

cassandra-stress help mixed says:

ratio([read=?][write=?][counter_write=?][counter_read=?]): Specify the ratios for operations to perform; e.g. (read=2,write=1) will perform 2 reads for each write
      read=? (default=1)                       Performs this many READ operations out of total
      write=? (default=1)                      Performs this many WRITE operations out of total
      counter_write=?                          Performs this many COUNTER_WRITE operations out of total
      counter_read=?                           Performs this many COUNTER_READ operations out of total

This means that counter_write and counter_read operations can be performed as well. Thus, the table keyspace1.counter1 is being used as well.

@muzarski
Copy link
Collaborator Author

The difference is that mixed should do 50/50 reads/writes ops.

The 50/50 reads/writes is the default behaviour, but can be configured via ratio parameter (so the tool performs operations on counter tables as well).

@muzarski muzarski requested a review from roydahan June 18, 2024 12:25
@muzarski
Copy link
Collaborator Author

ping @roydahan - I'd like to make sure we are on the same page so I can merge this.

@wprzytula wprzytula self-assigned this Jun 19, 2024
@roydahan
Copy link
Collaborator

Ok, I understand now.
So, as long as the default ratio is only "read,write" it should be fine.

roydahan
roydahan previously approved these changes Jun 19, 2024
@roydahan
Copy link
Collaborator

@soyacz I asked @muzarski to merge and release so we can retest the tablets one without even creating the table.
The "mixed" could support counters workload but you must explictily state a ratio that include counter_write, counter_read.

@roydahan
Copy link
Collaborator

@muzarski rethinking of it, AFAIR in c-s we don't create the tables in "mixed" command, it assumes that one already ran a "write" command before that in order to prepare for it.
So, unless it's an improvement we're adding on top of c-s, cql-stress shouldn't do it as well.

@roydahan roydahan dismissed their stale review June 19, 2024 13:30

still question about creating the table for mixed command

@muzarski
Copy link
Collaborator Author

muzarski commented Jun 19, 2024

@muzarski rethinking of it, AFAIR in c-s we don't create the tables in "mixed" command, it assumes that one already ran a "write" command before that in order to prepare for it. So, unless it's an improvement we're adding on top of c-s, cql-stress shouldn't do it as well.

Oh, that's interesting. But it would make sense, since mixed allows to execute read operation (which requires a populated DB). Let me verify this, and I'll adjust the logic properly for cql-stress.

muzarski added 2 commits June 19, 2024 17:50
List of tables created for each command:
- write -> `keyspace1.standard1` table
- counter_write -> `keyspace1.counter1` table
- user -> keyspace and table provided in user profile yaml file
- read/counter_read/mixed -> no table created
@muzarski
Copy link
Collaborator Author

v3:
List of tables created for each command:

  • write -> keyspace1.standard1
  • counter_write -> keyspace1.counter1
  • user -> based on profile
  • read/counter_read/mixed -> no table created

@roydahan Please confirm that it makes sense now.

@muzarski muzarski requested a review from roydahan June 19, 2024 16:00
@muzarski muzarski merged commit 7543997 into scylladb:master Jun 24, 2024
1 check passed
soyacz added a commit to soyacz/scylla-cluster-tests that referenced this pull request Jul 2, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86
soyacz added a commit to soyacz/scylla-cluster-tests that referenced this pull request Jul 2, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86
@fruch
Copy link

fruch commented Jul 2, 2024

@muzarski

now we don't create the schema...

but the code still tries to prepare statements on the non-existing keyspace/table..., which is even worse, mixed can't be use now...

TAG: loader_idx:4-cpu_idx:0-keyspace_idx:1
******************** Stress Settings ********************
Command:
  Type: mixed
  Count: -1
  Duration: 48000 SECONDS
  No Warmup: true
  Consistency Level: Quorum
  Serial Consistency Level: Serial
  Truncate: NEVER
  Target Uncertainty: not applicable
  Key Size (bytes): 10
  Counter Increment Distibution: FIXED(1)
Command ratios: {read=1,write=1}
Command clustering distribution: GAUSSIAN(1..10,mean=5.5,stdev=1.5)
Rate:
  Thread count: 300
  OpsPer Sec: 8750
  Coordinated-Omission-Fixed latencies: true
Mode:
  Compression: None
  Pool size: PerShard(1)
Node:
  Nodes: ["10.4.1.151", "10.4.3.241", "10.4.3.226"]
  Is White List: false
  Datacenter: None
Schema:
  Keyspace: keyspace1
  Replication Strategy Options: {"replication_factor": "3", "class": "NetworkTopologyStrategy"}
  Table Compression: None
  Table Compaction Options: {}
Column:
  Column names: ["C0", "C1", "C2", "C3", "C4", "C5", "C6", "C7"]
  Size distribution: FIXED(128)
Population:
  Partition key seed distribution: GAUSSIAN(1..650000000,mean=325000000,stdev=6500000)

Error: Failed to prepare benchmark

Caused by:
    0: Failed to prepare statement
    1: Database returned an error: The query is syntactically correct but invalid, Error message: unconfigured table counter1

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

@muzarski rethinking of it, AFAIR in c-s we don't create the tables in "mixed" command, it assumes that one already ran a "write" command before that in order to prepare for it.

So, unless it's an improvement we're adding on top of c-s, cql-stress shouldn't do it as well.

@fruch WDYT?

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

I can reverse the changes regarding mixed workload so it creates the schema.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

Looking at the code of original c-s:

    public void maybeCreateKeyspaces()
    {
        if (command.type == Command.WRITE || command.type == Command.COUNTER_WRITE)
            schema.createKeySpaces(this);
        else if (command.type == Command.USER)
            ((SettingsCommandUser) command).profile.maybeCreateSchema(this);
    }

Keyspace and tables are not created for mixed workloads. @fruch Please try to run the write workload firstly to create a schema. Or if this really annoying and you think that we should create schema during mixed command as well, please open an issue about it so we can discuss it there.

@fruch
Copy link

fruch commented Jul 2, 2024

Looking at the code of original c-s:

    public void maybeCreateKeyspaces()
    {
        if (command.type == Command.WRITE || command.type == Command.COUNTER_WRITE)
            schema.createKeySpaces(this);
        else if (command.type == Command.USER)
            ((SettingsCommandUser) command).profile.maybeCreateSchema(this);
    }

Keyspace and tables are not created for mixed workloads. @fruch Please try to run the write workload firstly to create a schema. Or if this really annoying and you think that we should create schema during mixed command as well, please open an issue about it so we can discuss it there.

We did run the write workload before, but the mixed workload shouldn't do any operations with counters, if we want counter there is a command for that.

Now tablets doesn't support counters, and we need that that read/write/mixed won't even create the counter schema, and not do any counter operations

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

Looking at the code of original c-s:

    public void maybeCreateKeyspaces()
    {
        if (command.type == Command.WRITE || command.type == Command.COUNTER_WRITE)
            schema.createKeySpaces(this);
        else if (command.type == Command.USER)
            ((SettingsCommandUser) command).profile.maybeCreateSchema(this);
    }

Keyspace and tables are not created for mixed workloads. @fruch Please try to run the write workload firstly to create a schema. Or if this really annoying and you think that we should create schema during mixed command as well, please open an issue about it so we can discuss it there.

We did run the write workload before, but the mixed workload shouldn't do any operations with counters, if we want counter there is a command for that.

Now tablets doesn't support counters, and we need that that read/write/mixed won't even create the counter schema, and not do any counter operations

Oh, I see. We try to prepare a statement operating on counter1 table. This is indeed a bug. I'll try to fix that ASAP.

soyacz added a commit to soyacz/scylla-cluster-tests that referenced this pull request Jul 18, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86
fruch pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86
mergify bot pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)

# Conflicts:
#	defaults/test_default.yaml
#	docker/cql-stress-cassandra-stress/image
mergify bot pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)

# Conflicts:
#	defaults/test_default.yaml
#	docker/cql-stress-cassandra-stress/image
mergify bot pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)
fruch pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)
fruch pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)

# Conflicts:
#	defaults/test_default.yaml
#	docker/cql-stress-cassandra-stress/image
fruch pushed a commit to scylladb/scylla-cluster-tests that referenced this pull request Jul 22, 2024
Updating cql-stress to latest version.
This version contains the fix for scylladb/cql-stress#86

(cherry picked from commit e518ae5)

# Conflicts:
#	defaults/test_default.yaml
#	docker/cql-stress-cassandra-stress/image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

c-s: Tool creates counter1 table when regular workloads are being run
4 participants