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

Fix latency reported by ideal consistency level monitoring #3391

Open
wants to merge 2 commits into
base: cassandra-4.1
Choose a base branch
from

Conversation

netudima
Copy link

@netudima netudima commented Jul 1, 2024

idealCLWriteLatency metric reported the worst response time instead of the time when ideal CL is satisfied.

Patch by Dmitry Konstantinov; reviewed by TBD for CASSANDRA-19651

idealCLWriteLatency metric reported the worst response time instead of the time when ideal CL is satisfied.

Patch by Dmitry Konstantinov; reviewed by TBD for CASSANDRA-19651
Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

This is great. Thank you for fixing this. One typo in a comment was all I found. Can you create branches for 4.0, 4.1, 5.0, and trunk and also remove the update to CHANGES.txt?

@@ -280,6 +289,8 @@ public void onFailure(InetAddressAndPort from, RequestFailureReason failureReaso

failureReasonByEndpoint.put(from, failureReason);

logFailureOrTimeoutToIdealCLDelegate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch looks like the failure path didn't decrement at all before?

I guess the only reason it gave some signal was because we would call expired explicitly if gossip thought the endpoint was down so it would be a non-zero metric.

Copy link
Author

Choose a reason for hiding this comment

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

yes, I think so, for prolonged node loss cases detected by gossip we have another path via expired() method.

Patch by Dmitry Konstantinov; reviewed by Ariel Weisberg for CASSANDRA-19651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants