Conversation
|
@athanatos This PR is to address your comment (#56067 (comment), #56067 (comment)) about refactoring global variables. Please take a look. |
|
Ok, I will get to this next week. |
|
|
||
| std::shared_lock l(glock); | ||
| state = new SampleDedupWorkerThread::SampleDedupGlobal( | ||
| chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold); |
There was a problem hiding this comment.
Never use raw new/delete without an extremely good reason. Use a unique_ptr.
There was a problem hiding this comment.
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.
| << dendl; | ||
|
|
||
| std::shared_lock l(glock); | ||
| state = new SampleDedupWorkerThread::SampleDedupGlobal( |
There was a problem hiding this comment.
Why is state declared outside of this function, couldn't it be a local std::unique_ptr?
| derr << "error fetching pool stats: " << cpp_strerror(ret) << dendl; | ||
| return -EINVAL; | ||
| ret = -EINVAL; | ||
| goto out; |
There was a problem hiding this comment.
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.
| derr << "stats can not find pool name: " << base_pool_name << dendl; | ||
| return -EINVAL; | ||
| ret = -EINVAL; | ||
| goto out; |
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>
fdcd5b8 to
a854567
Compare
|
|
||
| std::shared_lock l(glock); | ||
| state = new SampleDedupWorkerThread::SampleDedupGlobal( | ||
| chunk_dedup_threshold, sampling_ratio, report_period, fp_threshold); |
There was a problem hiding this comment.
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.
a854567 to
17ffdea
Compare
|
@athanatos Can you take a look to see if I addressed your comment? |
… a gobal variable Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com> Signed-off-by: Sungmin Lee <sung_min.lee@samsung.com>
17ffdea to
81382ac
Compare
|
I added needs-qa tag. |
…-ceph-dedup-daemon tool: renaming and refactoring global variables in ceph-dedup-daemon Reviewed-by: Samuel Just <sjust@redhat.com>
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
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e