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

Implement getPendingCompactions in HttpManagementProxy #1403

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Implement getPendingCompactions in HttpManagementProxy #1403

merged 3 commits into from
Oct 4, 2023

Conversation

adejanovski
Copy link
Contributor

@adejanovski adejanovski commented Sep 8, 2023 •
edited

Fixes #1382

Since metrics are now exposed on a different port than the mgmt-api itself, this required to go through the HttpMetricsProxy which got partially implemented for that need.
It is now able to pull metrics from the metrics endpoint and parse it into JmxStat objects (which will need to be later refactored to a new name).
Only tpstats are considered for now as the others are not needed for segments orchestration.

@adejanovski adejanovski linked an issue Sep 8, 2023 that may be closed by this pull request
Implement getPendingCompactions() in the HttpManagementProxy #1382
Closed
@adejanovski adejanovski marked this pull request as ready for review September 8, 2023 08:40
@adejanovski adejanovski mentioned this pull request Sep 8, 2023
Closed
3 tasks
@adejanovski adejanovski force-pushed the feature/get-pending-compactions-http branch 3 times, most recently from e60aa87 to 1ac6f0b Compare September 13, 2023 06:50
@emerkle826 emerkle826 force-pushed the feature/get-pending-compactions-http branch 2 times, most recently from 5f795cc to 19f8ce4 Compare September 19, 2023 17:55
@emerkle826 emerkle826 force-pushed the feature/get-pending-compactions-http branch from 19f8ce4 to 0b745d7 Compare September 27, 2023 20:30
@emerkle826 emerkle826 marked this pull request as draft September 29, 2023 03:21
@emerkle826 emerkle826 marked this pull request as ready for review September 29, 2023 05:36
@emerkle826 emerkle826 force-pushed the feature/get-pending-compactions-http branch from e06d105 to 8ba1b20 Compare September 29, 2023 05:45
@emerkle826 emerkle826 force-pushed the feature/get-pending-compactions-http branch from 8ba1b20 to 56cc73e Compare September 29, 2023 21:19
Miles-Garnsey
Miles-Garnsey requested changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

I've done a preliminary review, I'll look more tomorrow. I have a few requested changes, but most aren't really blocking.

Given the tests are passing, I'm almost happy to approve immediately, but given the size of this PR I want to take another look over it. I can see why it took a little while!

src/server/src/main/java/io/cassandrareaper/management/MetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/MetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/MetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java Outdated Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java Show resolved Hide resolved
@Miles-Garnsey Miles-Garnsey force-pushed the integration/http-managementproxy branch from d72a78c to e2d1998 Compare October 3, 2023 00:29
Miles-Garnsey
Miles-Garnsey requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

I mentioned on slack, but I'm having issues with reviewing this due to noise in the diff (I think the last rebase I did was responsible, need to discuss how to do this better next time).

Can we maybe rebase this against the current integration branch so I have clearer visibility?

src/server/src/test/java/io/cassandrareaper/acceptance/BasicSteps.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/MetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpCassandraManagementProxy.java Outdated Show resolved Hide resolved
@adejanovski
Implement getPendingCompactions in the HttpManagementProxy
8d7ef92 
Since metrics are now exposed on a different port than the mgmt-api itself, this required to go through the HttpMetricsProxy which got partially implemented for that need.
It is now able to pull metrics from the metrics endpoint and parse it into GenericMetrics.
Only tpstats are considered for now as the others are not needed for segments orchestration.

Use the new prefix filter for metrics

Fix failing ITs by switching to cassandra storage for HTTP ITs

Add tmate for debugging CI

Upgrade Cassandra versions for the mgmt-api tests

Disable metrics pull checks in http ITs

Add logging

implement dropped message and client request metrics

implement percent repaired method

fix tests, fix pending compactions

fix DroppedMessages test check, run all IT tests in CI

update Cassandra 4.0.1 to 4.0.6 for tests

Address Miles's comments

Fail fast when reading dropped messages metrics in ITs
@adejanovski adejanovski force-pushed the feature/get-pending-compactions-http branch from 8baf990 to 8d7ef92 Compare October 3, 2023 05:10
Miles-Garnsey
Miles-Garnsey requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

Some more feedback now that I can see what's happening here a bit better. I might have yet more tomorrow once I've gone through this in more depth again.

Sorry, this is a relatively large PR, and the amount of refactoring required (because we're making compromises to get it across the line) makes the logic much harder to make out.

src/server/src/main/java/io/cassandrareaper/management/http/HttpMetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpMetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpMetricsProxy.java Show resolved Hide resolved
src/server/src/main/java/io/cassandrareaper/management/http/HttpMetricsProxy.java
throw new ReaperException("Error collecting metrics", e);
}
return genericMetrics.stream()
.filter(metric -> metric.getMetricType().equals("ThreadPools"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: If there is no MetricType with value ThreadPools is this an error that should be thrown, or is it an expected condition which should return an empty list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if the answer is that this should throw, can you go back through each method to ensure that they all have consistent behaviour in that regard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the runtime behavior here, so I'd go with returning an empty list if no metric matches the requested metric type.

src/server/src/test/resources/cassandra-reaper-http-at.yaml Outdated
enabled: true

cassandra:
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is all of this new stuff required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, we need the driver configuration to connect to Cassandra.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still waiting on a response here.

@adejanovski adejanovski linked an issue Oct 3, 2023 that may be closed by this pull request
Implement the HttpMetricsProxy in Reaper #1396
Closed
3 tasks
Miles-Garnsey
Miles-Garnsey approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

@adejanovski is going to fix a duplicated block in the Reaper config for the acceptance tests, and also link to the metrics proxy refactoring ticket he raised.

Otherwise, this is approved.

src/server/src/test/resources/cassandra-reaper-http-at.yaml Outdated
enabled: true

cassandra:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still waiting on a response here.

@adejanovski
Copy link
Contributor Author

Here's the follow up ticket: #1423

@adejanovski adejanovski merged commit b9578bf into integration/http-managementproxy Oct 4, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@burmanm burmanm burmanm left review comments

@emerkle826 emerkle826 emerkle826 left review comments

@Miles-Garnsey Miles-Garnsey Miles-Garnsey 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.

Implement the HttpMetricsProxy in Reaper Implement getPendingCompactions() in the HttpManagementProxy
4 participants
@adejanovski @burmanm @emerkle826 @Miles-Garnsey