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

CASSANDRA-20149 Call JMX just once in nodetool listsnapshots #3750

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

smiklosovic
Copy link
Contributor

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

…call JMX twice in nodetool listsnapshots

This patch also opportunistically adds a new MBean method for SnapshotManager to
return true snapshot size for a particular snapshot.

patch by Stefan Miklosovic; reviewed by TBD for CASSANDRA-20149
@@ -74,14 +81,14 @@ public static void from(TableSnapshot details, TabularDataSupport result, Set<St
{
try
{
final String totalSize = FileUtils.stringifyFileSize(details.computeSizeOnDiskBytes());
Copy link
Contributor Author

@smiklosovic smiklosovic Dec 18, 2024

Choose a reason for hiding this comment

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

not sure why we are stressing so much that this has to be final ... doesnt matter at all.

* @param snapshotName name of snapshot in given keyspace and table to get true size of
* @return true size of all snapshots in given keyspace and table
*/
long getTrueSnapshotsSize(String keyspace, String table, String snapshotName);
Copy link
Contributor Author

@smiklosovic smiklosovic Dec 18, 2024

Choose a reason for hiding this comment

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

I noticed that a user does not have any way to get true disk space used for a particular snapshot except reading it from snapshot details (which is in human friendly format btw, not raw) which is overkill so I added this simple utility method for that.

table.add(indexNames.subList(0, indexNames.size() - 1).toArray(new String[indexNames.size() - 1]));
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the below logic deals with the fact that we do not want to have another column of "Raw true size" in the output of nodetool listsnapshots. All we are interested in are the numbers we sum up internally and display that in TrueDiskSpaceUsed at then end of the output.

We do have true sizes already returned, but they are in human friendly format and we can not sum these up.

Copy link
Contributor

@bbotella bbotella left a comment

Choose a reason for hiding this comment

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

Just a couple of nits. Other than that, LGTM.

+1 (non-binding)

Copy link
Contributor

@Mmuzaf Mmuzaf left a comment

Choose a reason for hiding this comment

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

+1

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.

3 participants