Circuit breaker by TousakaRin · Pull Request #413 · apache/brpc

TousakaRin · 2018-07-23T03:00:15Z

增加了CircuitBreaker：

每个Socket中都有一个CircuitBreaker对象
在ChannelOptions中新增enable_circuit_breaker开关
Socket发现CircuitBreaker::OnCallEnd返回false之后调用SetFailed，之后由healthy check thread调用 Reset 来复位。

还存在的问题：

只有main socket才需要circuitbreaker，每个socket都有一个circuitbreaker是否有些浪费。
PR中的默认参数还需要进一步调整。

jamesge · 2018-07-30T08:11:01Z

src/brpc/channel.h

    // Maximum: INT_MAX
    int max_retry;

+    // When the error rate of an endpoint is too high, deactivate the node. 


deactivate -> isolate, an endpoint -> a server node

jamesge · 2018-07-30T08:12:01Z

src/brpc/channel.h

    int max_retry;

+    // When the error rate of an endpoint is too high, deactivate the node. 
+    // Note that this deactive is GLOBAL, and the endpoint will become 


deactive -> isolation, endpoint -> node.
The node will become unavailable for all channels running in this process during the isolation.

jamesge · 2018-07-30T10:42:19Z

src/brpc/socket.h

        __attribute__ ((__format__ (__printf__, 3, 4)));
    static int SetFailed(SocketId id);

+    void FeedbackCircuitBreaker(int error_code, int64_t latency_us) {


这个接口没必要封装，在外部实现看得很清楚。

假如把CircuitBreaker放在SharedPart里的话，由于SharedPart的定义在cpp文件里，在Controller::Call::OnComplete里就拿不到CircuitBreaker了，会提示编译错误说SharedPart是incompleted class。
这样的话反而只能封装接口了

jamesge · 2018-07-30T10:46:15Z

src/brpc/socket.h

    butil::Mutex _stream_mutex;
    std::set<StreamId> *_stream_set;
+
+    CircuitBreaker _circuit_breaker;


放的位置不对，应该在SharedPart里面。这是为了让连接池模式下(Controller::Call::peer_id代表的）主链接和(Controller::Call::sending_sock代表的）实际连接方便地共享状态。注意：其他链接方式当然也会用到SharedPart，只是主链接就是sending_sock

jamesge · 2018-07-30T10:57:32Z

src/brpc/circuit_breaker.h

+        return true;
+    }
+
+    butil::DoublyBufferedData<ErrRecorderList> _recorders;


这个类有点over-design了，DBD和ErrRecorderList都是不必要的。用户就是实现一个接口OnCallEnd，这个对象会new出来设置在Socket.SharedPart中，健康检查前(Socket.WaitAndReset)中被delete掉。长短两个窗口就是inline地写在代码中（当然一些函数可以共用），而不需要用vector管理。

”这个对象会new出来设置在Socket.SharedPart中” 是指CircuitBreaker对象需要new出来放在SharedPart中吗，我觉得一直放在SharedPart里，然后在WaiAndReset里调一下CircuitBreaker::Reset就可以了吧

feng-y · 2018-08-16T15:42:52Z

src/brpc/circuit_breaker.h

+#ifndef BRPC_CIRCUIT_BREAKER_H
+#define BRPC_CIRCUIT_BREAKER_H
+
+#include "butil/containers/doubly_buffered_data.h"


这个头文件是多余的吧

jamesge · 2018-08-30T03:59:39Z

src/brpc/controller.h

    // Some of them are copied from `Channel' which might be destroyed
    // after CallMethod.
    int _max_retry;
+    bool _enable_circuit_breaker;


建议加到Controller::FLAGS_xxx里去。

jamesge · 2018-08-30T04:00:34Z

src/brpc/socket.cpp

+void Socket::FeedbackCircuitBreaker(int error_code, int64_t latency_us) {
+    if (!GetOrNewSharedPart()->circuit_breaker.OnCallEnd(error_code, latency_us)) {
+        LOG(ERROR) 
+            << "Socket[" << *this << "] deactivted by circuit breaker";


这个换行很奇怪，行宽不超过100都ok。

jamesge · 2018-08-30T04:04:58Z

src/brpc/controller.cpp

    }
+    if (enable_circuit_breaker) {
+        SocketUniquePtr sock;
+        if (Socket::Address(peer_id, &sock) == 0) {


上面就有sending_sock，不用address了。现在放sharedpart里，sending_sock是和peer_id对应的(mainsock)共享的，所以就能访问到。注意sending_sock会被置空，这块代码位置可以调整一下。

jamesge · 2018-08-30T04:15:24Z

src/brpc/circuit_breaker.cpp

+
+int64_t CircuitBreaker::EmaErrorRecorder::UpdateLatency(int64_t latency) {
+    while (true) {
+        int64_t ema_latency = _ema_latency.load(butil::memory_order_relaxed);


load放循环外。失败的case会更新ema_latency的。另外这里写成do while更好。

jamesge · 2018-08-30T04:20:50Z

src/brpc/circuit_breaker.cpp

+    //Ordinary response
+    while (true) {
+        int64_t ema_error_cost = 
+            _ema_error_cost.load(butil::memory_order_relaxed);


load放循环外，失败cas会更新参数。

jamesge · 2018-08-30T04:22:37Z

src/brpc/circuit_breaker.cpp

+    int sample_count = _sample_count.fetch_add(1, butil::memory_order_relaxed);
+    bool init_completed = _init_completed.load(butil::memory_order_acquire);
+    if (!init_completed && sample_count >= _window_size) {
+        _init_completed.store(true, butil::memory_order_release);


没看出这个init_completed意义在哪，每次判下_sample_count和_window_size关系不就行了？

jamesge · 2018-08-30T04:24:18Z

src/brpc/circuit_breaker.cpp

+DEFINE_int32(circuit_breaker_min_error_cost_us, 100,
+    "The minimum error_cost, when the ema of error cost is less than this "
+    "value, it will be set to zero.");
+DEFINE_int32(circuit_breaker_max_failed_latency_mutliple, 2,


mutliple -> multiple。这主要是针对超时的把，2倍够么？

gydong · 2018-09-17T06:28:47Z

src/brpc/socket.cpp


+void Socket::FeedbackCircuitBreaker(int error_code, int64_t latency_us) {
+    if (!GetOrNewSharedPart()->circuit_breaker.OnCallEnd(error_code, latency_us)) {
+        LOG(ERROR) << "Socket[" << *this << "] deactivted by circuit breaker";


deactivated 改为 isolated 以保持用词统一

jamesge · 2018-09-17T06:01:08Z

src/brpc/controller.h

-        SocketId peer_id;          // main server id
-        int64_t begin_time_us;     // sent real time.
+        SocketId peer_id;            // main server id
+        int64_t begin_time_us;       // sent real time.


//最好和上面对齐

jamesge · 2018-09-17T06:07:15Z

src/brpc/circuit_breaker.cpp

+    "the average latency of the success requests.");
+DEFINE_int32(circuit_breaker_min_isolation_duration_ms, 100,
+    "Minimum isolation duration in milliseconds");
+DEFINE_int32(circuit_breaker_max_isolation_duration_ms, 300000,


太长了，可能会产生badcase，先取30000吧

jamesge · 2018-09-17T06:22:59Z

src/brpc/circuit_breaker.cpp

+        if (now_time_ms - _last_reset_time_ms < max_isolation_duration_ms) {
+            isolation_duration_ms *= 2;
+            isolation_duration_ms = 
+                std::min(isolation_duration_ms, max_isolation_duration_ms);


两行改成min第一个参数*2就好了。

src/brpc/circuit_breaker.cpp

jamesge · 2018-09-17T06:51:18Z

src/brpc/circuit_breaker.cpp

+DEFINE_int32(circuit_breaker_min_error_cost_us, 500,
+    "The minimum error_cost, when the ema of error cost is less than this "
+    "value, it will be set to zero.");
+DEFINE_int32(circuit_breaker_max_failed_latency_mutilple, 2,


mutilple -> mutiple，全部搜一下吧

gydong · 2018-09-17T23:53:35Z

src/brpc/socket.h

 #include "brpc/options.pb.h"              // ConnectionType
 #include "brpc/socket_id.h"               // SocketId
 #include "brpc/socket_message.h"          // SocketMessagePtr
+#include "brpc/circuit_breaker.h"         // CircuitBreaker


此处不需要include？

gydong · 2018-09-18T00:48:28Z

src/brpc/socket.cpp

+void Socket::FeedbackCircuitBreaker(int error_code, int64_t latency_us) {
+    if (!GetOrNewSharedPart()->circuit_breaker.OnCallEnd(error_code, latency_us)) {
+        LOG(ERROR) << "Socket[" << *this << "] deactivted by circuit breaker";
+        SetFailed();


SetFailed(peer_id)

src/brpc/socket.cpp

…uests

…ing_sock is NULL

…health check will not be started.

…ck is passed.

jamesge mentioned this pull request Jul 30, 2018

下游服务熔断 #176

Closed

jamesge reviewed Jul 30, 2018

View reviewed changes

jamesge mentioned this pull request Aug 2, 2018

channel无法剔除故障节点，疑似bug #348

Closed

feng-y reviewed Aug 16, 2018

View reviewed changes

TousakaRin force-pushed the circuit_breaker branch from 25504df to 7f80dc4 Compare August 28, 2018 08:39

jamesge reviewed Aug 30, 2018

View reviewed changes

TousakaRin force-pushed the circuit_breaker branch 6 times, most recently from 655fbb0 to 6e5a863 Compare August 31, 2018 06:44

TousakaRin force-pushed the circuit_breaker branch 2 times, most recently from 373297e to 2ae2559 Compare September 9, 2018 09:54

gydong reviewed Sep 17, 2018

View reviewed changes

jamesge reviewed Sep 17, 2018

View reviewed changes

gydong reviewed Sep 17, 2018

View reviewed changes

gydong reviewed Sep 18, 2018

View reviewed changes

src/brpc/socket.cpp Show resolved Hide resolved

TousakaRin and others added 10 commits September 18, 2018 10:12

Add circuit breaker for channel

db34f96

Modify the default values of window_size

499340a

1.Simplified the design of CircuitBreaker. 2. modified the comments

ffca94c

Keep the SharedPart' is defined in the cpp file.

4da9125

Fix the isolation caused by the excessive latency of the timeout req…

3706336

…uests

Replace _enable_circuit_breaker with FALGS_ENABLED_CIRCUIT_BREAKER

f9d80bd

Fix indent

75f768a

Fix typo, delete member _init_completed

f042bb6

change default value of glags for circuit_breaker

df273c6

Fix coredump in unittest

16c6d5d

TousakaRin added 12 commits September 18, 2018 10:12

change default value of glags for circuit_breaker

18f29e3

merge code from upstream

fc5b4ab

fix coredump

cc37c96

fix bug in unittest

d1e6939

Use the peer_id to call the FeedbackCircuitBreaker only when the send…

7f20db9

…ing_sock is NULL

Increase the duration of isolation

6109643

fix bug: Isolation cannot be triggered when ema_latency is 0

2dec1be

Fix bug: a typo that could lead to a coredump

a92ee0b

delete unnecessary include

9b53cc0

fix typo, make logic clearer

89f462d

fix code style, make the logic clearer

79ab26f

Fix the bug that in the pooled mode, after the isolation occurs, the …

036a8a4

…health check will not be started.

TousakaRin force-pushed the circuit_breaker branch from b877c0d to 036a8a4 Compare September 18, 2018 02:14

TousakaRin added 5 commits September 18, 2018 10:17

fix codes tyle

e8dcc94

Delay the call timing of Circuit::Reset and wait until the health che…

09b75b5

…ck is passed.

Remove EmaRecorder::_broken

1ff59ab

1. Remove cacheline alignment 2.make the logic clearer

9145cb8

Remove unnecessary temporary variables

620811e

jamesge merged commit ce50327 into apache:master Sep 19, 2018

TousakaRin mentioned this pull request Jan 4, 2022

1.0.0 庆祝发版并感谢贡献者活动 #1652

Closed

Conversation

TousakaRin commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants