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
CASSANDRA-16183 #1
base: trunk
Are you sure you want to change the base?
Conversation
client_request_metrics_test.py
Outdated
global_diff, cl_diff = self.validate_nominal('Write', | ||
WriteMetrics, | ||
SimpleStatement(f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0)", consistency_level=CL.ONE), | ||
query_count) | ||
|
||
assert global_diff['MutationSizeHistogram.Count'] == query_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could easily test all valid consistency levels here, instead on only CL.ONE
:
global_diff, cl_diff = self.validate_nominal('Write', | |
WriteMetrics, | |
SimpleStatement(f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0)", consistency_level=CL.ONE), | |
query_count) | |
assert global_diff['MutationSizeHistogram.Count'] == query_count | |
query = f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0)" | |
for cl in (CL.LOCAL_ONE, CL.ONE, CL.TWO, CL.ALL, CL.ANY, CL.LOCAL_QUORUM): | |
global_diff, cl_diff = self.validate_nominal('Write', | |
WriteMetrics, | |
SimpleStatement(query, consistency_level=cl), | |
query_count) |
client_request_metrics_test.py
Outdated
self.validate_nominal('Read', | ||
ClientRequestMetrics, | ||
SimpleStatement(f"SELECT k FROM {KEYSPACE}.{TABLE} WHERE k=0 AND c=0", consistency_level=CL.LOCAL_ONE), | ||
query_count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could also test more consistency levels:
self.validate_nominal('Read', | |
ClientRequestMetrics, | |
SimpleStatement(f"SELECT k FROM {KEYSPACE}.{TABLE} WHERE k=0 AND c=0", consistency_level=CL.LOCAL_ONE), | |
query_count) | |
query = f"SELECT k FROM {KEYSPACE}.{TABLE} WHERE k=0 AND c=0" | |
for cl in (CL.LOCAL_ONE, CL.ONE, CL.TWO, CL.ALL, CL.LOCAL_QUORUM): | |
self.validate_nominal('Read', | |
ClientRequestMetrics, | |
SimpleStatement(query, consistency_level=cl), | |
query_count) |
|
||
def write_failures_variant(self, scope, constraint, query_count, validator, metric_class): | ||
validator = self.validate_metric if validator is None else validator | ||
query_cl = CL.ONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could pass the consistency level as an argument so the two callers can test different CLs.
return diff | ||
|
||
def write_unavailables(self): | ||
query_cl = CL.THREE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a short comment indicating that the writes are unavailable because the replication factor is 2.
client_request_metrics_test.py
Outdated
for _ in range(query_count): | ||
try: | ||
self.session.execute(statement) | ||
# self.session.execute(statement, execution_profile=exe_profile, timeout=20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a leftover from previous changes
client_request_metrics_test.py
Outdated
def cas_write_failures(self): | ||
query_count = 2 | ||
diff = self.write_failures_variant('CASWrite', f"WHERE k={new_key()} AND c=0 IF v!=0", | ||
query_count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: misaligned
query_count, | |
query_count, |
client_request_metrics_test.py
Outdated
'Unavailables', | ||
Unavailable | ||
) | ||
self.start_cluster() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use just node2.start
. The JVM properties that we pass in start_cluster
can be placed in a static property to be reused here. If we do so there would be a single call to the start_cluster
method in setup_once
, so we could get rid of start_cluster
. WDYT?
client_request_metrics_test.py
Outdated
# The way we're failing writes causes a StorageProxy::cas to throw before the metric is | ||
# incremented on each request after the first one. We find the previous ballot in-progress and fail trying to | ||
# commit it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# The way we're failing writes causes a StorageProxy::cas to throw before the metric is | |
# incremented on each request after the first one. We find the previous ballot in-progress and fail trying to | |
# commit it. | |
# The way we're failing writes causes a StorageProxy::cas to throw before the metric is incremented on each | |
# request after the first one. We find the previous ballot in-progress and fail trying to commit it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have finished my review. The new tests look good to me and I have only added a few minor suggestions.
client_request_metrics_test.py
Outdated
CASClientWriteRequestMetrics, | ||
SimpleStatement(f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0) IF NOT EXISTS", | ||
consistency_level=CL.ONE), | ||
2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: alignement
CASClientWriteRequestMetrics, | |
SimpleStatement(f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0) IF NOT EXISTS", | |
consistency_level=CL.ONE), | |
2) | |
CASClientWriteRequestMetrics, | |
SimpleStatement(f"INSERT INTO {KEYSPACE}.{TABLE} (k,c) VALUES (0,0) IF NOT EXISTS", | |
consistency_level=CL.ONE), | |
2) |
client_request_metrics_test.py
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client_request_metrics_test.py
Outdated
|
||
def validate(self): | ||
assert self.values['RateUnit'] == 'events/second' | ||
for k,v in self.values.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missed space
for k,v in self.values.items(): | |
for k, v in self.values.items(): |
client_request_metrics_test.py
Outdated
assert all(isinstance(i, int) for i in l), k | ||
|
||
|
||
k = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: we could rename k
to something like current_key
or last_key
to avoid name shadowing on the methods before.
client_request_metrics_test.py
Outdated
|
||
def is_histo_list(k, l): | ||
# since these values change on sampling, we can only generally verify it takes the proper form | ||
# there is a dedicated test to make sure the histogram behaves as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also indicate here which test
f38564e
to
f73712b
Compare
No description provided.