Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

feat: add an interface to get read_throttling_reject_count perf counter #940

Closed
wants to merge 4 commits into from
Closed

feat: add an interface to get read_throttling_reject_count perf counter #940

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2181bc
add an interface
Smityz Oct 20, 2021
f59f166
refactor
Smityz Oct 20, 2021
05f4e05
fix
Smityz Oct 21, 2021
360760f
Update replication_app_base.cpp
Smityz Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/dsn/dist/replication/replication_app_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@ class replication_app_base : public replica_base
replica_init_info _info;

explicit replication_app_base(::dsn::replication::replica *replica);

dsn::perf_counter *get_counter_recent_read_throttling_reject_count();
Copy link
Contributor

Choose a reason for hiding this comment

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

why didn't you return the smart pointer?

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 original perf_counter is a raw point, passed by _counter_recent_read_throttling_reject_count.get()

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @levy5307, it's better to avoid raw pointer and use smart pointer, although the original value is raw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It's meaningless to wrap a raw point by shared_ptr

  2. I think it's no risk there because replica has the longer lifetime than _app
    ref: https://github.com/XiaoMi/rdsn/blob/master/src/replica/replica.h#L568
    https://github.com/XiaoMi/rdsn/blob/master/src/replica/replica.h#L488

  3. If I refactor raw point to shared_ptr here, it would be a big change, due to there are many codes using define, and I have to replace -> to ->operator->()
    ref: https://github.com/XiaoMi/rdsn/blob/master/src/replica/replica_throttle.cpp#L30

Copy link
Contributor

Choose a reason for hiding this comment

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

I have another question, why would you like to pass this perf-counter to pegasus?

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 throttling_controller works on Pegasus, so must use the same perf_counter to collect read reject count

Copy link
Contributor

Choose a reason for hiding this comment

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

I have another question, why would you like to pass this perf-counter to pegasus?

+1, the function put here seems to be strange

};

} // namespace replication
Expand Down
4 changes: 3 additions & 1 deletion src/replica/replica.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

#include <dsn/perf_counter/perf_counter_wrapper.h>
#include <dsn/dist/replication/replica_base.h>
#include <dsn/dist/replication/replication_app_base.h>

#include "common/replication_common.h"
#include "mutation.h"
Expand All @@ -62,7 +63,6 @@ class access_controller;
} // namespace security
namespace replication {

class replication_app_base;
class replica_stub;
class replica_duplicator_manager;
class replica_backup_manager;
Expand Down Expand Up @@ -467,6 +467,8 @@ class replica : public serverlet<replica>, public ref_counter, public replica_ba
friend class replica_disk_migrator;
friend class replica_disk_test;
friend class replica_disk_migrate_test;
friend dsn::perf_counter *
replication_app_base::get_counter_recent_read_throttling_reject_count();

// replica configuration, updated by update_local_configuration ONLY
replica_configuration _config;
Expand Down
9 changes: 9 additions & 0 deletions src/replica/replication_app_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,5 +603,14 @@ ::dsn::error_code replication_app_base::update_init_info_ballot_and_decree(repli
_info.init_offset_in_private_log,
r->last_durable_decree());
}

dsn::perf_counter *replication_app_base::get_counter_recent_read_throttling_reject_count()
{
;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

dassert(_replica->_counter_recent_read_throttling_reject_count.get(),
"_counter_recent_read_throttling_reject_count is null");

return _replica->_counter_recent_read_throttling_reject_count.get();
}
} // namespace replication
} // namespace dsn