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

Add cron to recalculate db size #2217

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Conversation

kinoute
Copy link
Contributor

@kinoute kinoute commented Apr 2, 2024

Following RocksLabs/kvrocks_exporter#25, I was wondering if there is any way to automatically recalculate the db size instead of doing it manually through dbsize scan with the redis-cli. Because without it, the panel on the Grafana dashboard doesn't make any sense without manual intervention, same for the info command through the CLI or redis client.

I thought it would be nice to add a cronjob that could run periodically to run this command on all namespaces to have up-to-date estimated keys number (default: disabled).

This is my first PR here and in C++, I hope I didn't make any mess.

PragmaTwice
PragmaTwice previously approved these changes Apr 2, 2024
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

It generally looks good to me.

@git-hulk
Copy link
Member

git-hulk commented Apr 3, 2024

Generally looks good, you can fix the tidy issue according to https://github.com/apache/kvrocks/actions/runs/8526319228/job/23369018645?pr=2217#step:7:1294

jihuayu
jihuayu previously approved these changes Apr 3, 2024
Copy link
Member

@jihuayu jihuayu left a comment

Choose a reason for hiding this comment

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

Great feature, thank you.

@kinoute kinoute dismissed stale reviews from jihuayu and PragmaTwice via 532b929 April 3, 2024 09:15
@kinoute
Copy link
Contributor Author

kinoute commented Apr 3, 2024

@git-hulk I pre-allocated the namespaces variable, I don't know if it's alright. Any idea why clang-tidy doesn't raise the same error for the command namespace get * implementation? My PR is based on this: https://github.com/apache/kvrocks/blob/unstable/src/commands/cmd_server.cc#L100-L107

  std::vector<std::string> namespaces;
  auto tokens = srv->GetNamespace()->List();
  for (auto &token : tokens) {
    namespaces.emplace_back(token.second);  // namespace
    namespaces.emplace_back(token.first);   // token
  }
  namespaces.emplace_back(kDefaultNamespace);
  namespaces.emplace_back(config->requirepass);

@git-hulk
Copy link
Member

git-hulk commented Apr 3, 2024

@git-hulk I pre-allocated the namespaces variable, I don't know if it's alright. Any idea why clang-tidy doesn't raise the same error for the command namespace get * implementation? My PR is based on this: https://github.com/apache/kvrocks/blob/unstable/src/commands/cmd_server.cc#L100-L107

  std::vector<std::string> namespaces;
  auto tokens = srv->GetNamespace()->List();
  for (auto &token : tokens) {
    namespaces.emplace_back(token.second);  // namespace
    namespaces.emplace_back(token.first);   // token
  }
  namespaces.emplace_back(kDefaultNamespace);
  namespaces.emplace_back(config->requirepass);

@kinoute That's right to reserve the space for namespaces. And why it doesn't raise an error in the previous codes, is because it's added before introducing the linter.

@git-hulk git-hulk merged commit 9dc0dc2 into apache:unstable Apr 3, 2024
30 checks passed
@git-hulk
Copy link
Member

git-hulk commented Apr 3, 2024

@kinoute Thanks for your contribution.

Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
49.5% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@kinoute kinoute deleted the cron-dbsize-scan branch April 3, 2024 15:25
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.

None yet

4 participants