scylladb / cql-stress

8 stars 4 forks source link

c-s: `-mode` option #69

Closed muzarski closed 3 months ago

muzarski commented 4 months ago

Motivation

Cassandra-stress supports -mode CLI option which configures the connection with database.

Changes

In this PR we introduce only a part of CLI parameters supported by -mode option, which are broadly used in SCT:

I decided not to introduce following parameters - I don't think they are any useful + they are not used in SCT at all:

piodul commented 4 months ago
  • cql3, native - dummy parameters used in SCT. Cassandra-Stress accepts simplenative and thrift modes which are obviously not supported by Rust driver. These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT.

I'm not sure we should plainly ignore them. If we have some tests in SCT which use thrift then I think that cql-stress should return an error if one tries to pass this option - so that a test does not accidentally run with the CQL protocol.

piodul commented 4 months ago
  • connectionsPerHost=, connectionsPerShard= - the latter one is a bonus parameter introduced in cql-stress - previously introduced in

How does the original c-s with our fork of the driver treat connectionsPerHost, does it work as one would imagine or is it ignored? Do we use this parameter in SCT intentionally?

fruch commented 4 months ago
  • cql3, native - dummy parameters used in SCT. Cassandra-Stress accepts simplenative and thrift modes which are obviously not supported by Rust driver. These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT.

I'm not sure we should plainly ignore them. If we have some tests in SCT which use thrift then I think that cql-stress should return an error if one tries to pass this option - so that a test does not accidentally run with the CQL protocol.

we don't use thrift in SCT, it's deprecated for quite some time, I don't think it would ever be used.

fruch commented 4 months ago
  • connectionsPerHost=, connectionsPerShard= - the latter one is a bonus parameter introduced in cql-stress - previously introduced in

How does the original c-s with our fork of the driver treat connectionsPerHost, does it work as one would imagine or is it ignored? Do we use this parameter in SCT intentionally?

it works as describe, and we use it in SCT, in two test cases, in one of them to generate lots many connection as possible.

piodul commented 4 months ago
  • cql3, native - dummy parameters used in SCT. Cassandra-Stress accepts simplenative and thrift modes which are obviously not supported by Rust driver. These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT.

I'm not sure we should plainly ignore them. If we have some tests in SCT which use thrift then I think that cql-stress should return an error if one tries to pass this option - so that a test does not accidentally run with the CQL protocol.

we don't use thrift in SCT, it's deprecated for quite some time, I don't think it would ever be used.

If that is the case, then it shouldn't be a problem to return an error when thrift is provided. Maybe we can skip adding this parameter altogether.

In fact, I checked out the code of the PR and neither thrift nor simplenative appears anywhere in the code. So it looks like it's not that we parse the parameters but ignore them, but rather we decided to ignore the parameters by not implementing them. I think this is the right approach, but the wording in the PR description was confusing.

muzarski commented 4 months ago
  • cql3, native - dummy parameters used in SCT. Cassandra-Stress accepts simplenative and thrift modes which are obviously not supported by Rust driver. These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT.

I'm not sure we should plainly ignore them. If we have some tests in SCT which use thrift then I think that cql-stress should return an error if one tries to pass this option - so that a test does not accidentally run with the CQL protocol.

we don't use thrift in SCT, it's deprecated for quite some time, I don't think it would ever be used.

If that is the case, then it shouldn't be a problem to return an error when thrift is provided. Maybe we can skip adding this parameter altogether.

In fact, I checked out the code of the PR and neither thrift nor simplenative appears anywhere in the code. So it looks like it's not that we parse the parameters but ignore them, but rather we decided to ignore the parameters by not implementing them. I think this is the right approach, but the wording in the PR description was confusing.

Yes, sorry for the confusion. The part: These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT. of the description relate to cql3 and native parameters which are ignored - not the thrift and simplenative.

piodul commented 4 months ago

Yes, sorry for the confusion. The part: These two parameters are simply ignored but introduced so cassandra-stress commands can be easily replaced with cql-stress commands in SCT. of the description relate to cql3 and native parameters which are ignored - not the thrift and simplenative.

Then please rephrase the paragraph to be less ambiguous about this.

muzarski commented 4 months ago

v2: rebased on top of master

muzarski commented 4 months ago

v3: removed underscores from variable names

muzarski commented 3 months ago

v4: addressed review comment

muzarski commented 3 months ago

@piodul can we merge this one?