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

tool: renaming and refactoring global variables in ceph-dedup-daemon #57223

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

tool: renaming and refactoring global variables in ceph-dedup-daemon #57223

wants to merge 3 commits into from
+48 −46

Conversation

myoungwon
Copy link
Member

@myoungwon myoungwon commented May 2, 2024 •
edited

Follow-up: #56067

This is 2nd PR to rename and refactor globals.

Signed-off-by: Myoungwon Oh myoungwon.oh@samsung.com

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@myoungwon myoungwon requested a review from athanatos May 2, 2024 14:16
@myoungwon
Copy link
Member Author

@athanatos This PR is to address your comment (#56067 (comment), #56067 (comment)) about refactoring global variables. Please take a look.

@athanatos
Copy link
Contributor

Ok, I will get to this next week.

athanatos
athanatos requested changes May 20, 2024
View reviewed changes
src/tools/ceph_dedup/ceph_dedup_daemon.cc
@@ -662,16 +668,14 @@ int make_crawling_daemon(const po::variables_map &opts)
<< ")"
<< dendl;

std::shared_lock l(glock);
state = new SampleDedupWorkerThread::SampleDedupGlobal(
chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Never use raw new/delete without an extremely good reason. Use a unique_ptr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I didn't look closely enough -- you don't even need a std::unique_ptr. You can just declare it directly as

  SampleDedupWorkerThread::SampleDedupGlobal state(
    chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold);

since it's a simple, concrete structure.

src/tools/ceph_dedup/ceph_dedup_daemon.cc Outdated
@@ -662,16 +668,14 @@ int make_crawling_daemon(const po::variables_map &opts)
<< ")"
<< dendl;

std::shared_lock l(glock);
state = new SampleDedupWorkerThread::SampleDedupGlobal(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is state declared outside of this function, couldn't it be a local std::unique_ptr?

src/tools/ceph_dedup/ceph_dedup_daemon.cc Outdated
@@ -717,32 +721,38 @@ int make_crawling_daemon(const po::variables_map &opts)
ret = rados.get_pool_stats(pool_names, stats);
if (ret < 0) {
derr << "error fetching pool stats: " << cpp_strerror(ret) << dendl;
return -EINVAL;
ret = -EINVAL;
goto out;
Copy link
Contributor

Choose a reason for hiding this comment

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

This goto is here so that you can call invoke delete state. Once you've switched it to a unique_ptr, this can go back to being a normal return.

src/tools/ceph_dedup/ceph_dedup_daemon.cc Outdated
}
if (stats.find(base_pool_name) == stats.end()) {
derr << "stats can not find pool name: " << base_pool_name << dendl;
return -EINVAL;
ret = -EINVAL;
goto out;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@myoungwon
tool/ceph_dedup: refactor globals to put them into automatic variable
111c94e 
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
@myoungwon
tool/ceph_dedup: rename make_crawling_daemon to run_crawling_daemon
54516b7 
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
@myoungwon myoungwon force-pushed the wip-rename-global-values-ceph-dedup-daemon branch from fdcd5b8 to a854567 Compare May 21, 2024 00:41
athanatos
athanatos requested changes May 21, 2024
View reviewed changes
src/tools/ceph_dedup/ceph_dedup_daemon.cc
@@ -662,16 +668,14 @@ int make_crawling_daemon(const po::variables_map &opts)
<< ")"
<< dendl;

std::shared_lock l(glock);
state = new SampleDedupWorkerThread::SampleDedupGlobal(
chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I didn't look closely enough -- you don't even need a std::unique_ptr. You can just declare it directly as

  SampleDedupWorkerThread::SampleDedupGlobal state(
    chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold);

since it's a simple, concrete structure.

@myoungwon myoungwon force-pushed the wip-rename-global-values-ceph-dedup-daemon branch from a854567 to 17ffdea Compare May 21, 2024 01:18
@myoungwon
Copy link
Member Author

@athanatos Can you take a look to see if I addressed your comment?

@myoungwon
tool/ceph_dedup: move signal_handler into SampleDedupGlobal to remove…
81382ac 
… a gobal variable

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Signed-off-by: Sungmin Lee <sung_min.lee@samsung.com>
@myoungwon myoungwon force-pushed the wip-rename-global-values-ceph-dedup-daemon branch from 17ffdea to 81382ac Compare May 21, 2024 16:09
@myoungwon
Copy link
Member Author

I added needs-qa tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@athanatos athanatos athanatos approved these changes

Assignees
No one assigned
Labels
core needs-qa tools
Projects
None yet
Milestone
No milestone
3 participants
@myoungwon @athanatos @batrick