membphis commented on code in PR #13764:
URL: https://github.com/apache/apisix/pull/13764#discussion_r3683090408
##########
apisix/plugins/api-breaker.lua:
##########
@@ -195,8 +383,145 @@ function _M.access(conf, ctx)
return
end
+-- Ratio-based circuit breaker
+local function ratio_based_access(conf, ctx)
+ -- Check and reset sliding window first to ensure consistent state
+ check_and_reset_window(ctx, conf)
-function _M.log(conf, ctx)
+ local current_state = get_circuit_breaker_state(ctx)
+ local current_time = ngx.time()
+
+ -- Handle OPEN state
+ if current_state == OPEN then
+ local last_change_key = gen_last_state_change_key(ctx)
+ local last_change_time, err = shared_buffer:get(last_change_key)
+ if err then
+ core.log.warn("failed to get last change time: ", err)
+ return conf.break_response_code or 503,
+ conf.break_response_body or "Service temporarily
unavailable"
+ end
+
+ local wait_duration = conf.max_breaker_sec or 300
+ if last_change_time and (current_time - last_change_time) >=
wait_duration then
+ -- Use atomic operation to ensure only one request transitions to
HALF_OPEN
+ local transition_key = "cb-transition-" ..
core.request.get_host(ctx) .. ctx.var.uri
+ local transition_success
+ transition_success, err = shared_buffer:add(transition_key, 1, 1)
+
+ if err then
+ core.log.warn("failed to add transition lock: ", err)
+ end
+
+ if transition_success then
+ -- Transition to HALF_OPEN
+ set_circuit_breaker_state(ctx, HALF_OPEN)
+ -- Reset half-open counters
+ shared_buffer:set(gen_half_open_calls_key(ctx), 0)
+ shared_buffer:set(gen_half_open_success_key(ctx), 0)
+ core.log.info("Circuit breaker transitioned from OPEN to
HALF_OPEN")
+
+ -- Clean up transition lock
+ shared_buffer:delete(transition_key)
Review Comment:
P1 — `half_open_max_calls` is not enforced under concurrency. The request
that transitions `OPEN -> HALF_OPEN` returns here without incrementing
`half_open_calls`, and deleting the lock immediately lets requests that already
observed `OPEN` acquire it again. The regression test with a limit of 2
currently expects three 200 responses. Please make the transition atomic, count
the transition request, and decide recovery only after all admitted probes
complete.
##########
apisix/plugins/api-breaker.lua:
##########
@@ -133,13 +267,67 @@ local _M = {
schema = schema,
}
-
function _M.check_schema(conf)
return core.schema.check(schema, conf)
end
+-- Circuit breaker state management functions
+local function get_circuit_breaker_state(ctx)
+ local state_key = gen_state_key(ctx)
+ local state, err = shared_buffer:get(state_key)
+ if err then
+ core.log.warn("failed to get circuit breaker state: ", err)
+ return CLOSED
+ end
+ return state or CLOSED
+end
+
+local function set_circuit_breaker_state(ctx, state)
+ local state_key = gen_state_key(ctx)
+ local last_change_key = gen_last_state_change_key(ctx)
+ local current_time = ngx.time()
-function _M.access(conf, ctx)
+ shared_buffer:set(state_key, state)
+ shared_buffer:set(last_change_key, current_time)
+
+ core.log.info("Circuit breaker state changed to: ", state, " at: ",
current_time)
+end
+
+-- Sliding window management
+local function reset_sliding_window(ctx, current_time, window_size)
+ local window_start_key = gen_window_start_time_key(ctx)
+ local total_requests_key = gen_total_requests_key(ctx)
+ local unhealthy_key = gen_unhealthy_key(ctx)
+
+ shared_buffer:set(window_start_key, current_time)
+ shared_buffer:set(total_requests_key, 0)
Review Comment:
P1 — This is a tumbling-window reset, not a sliding window, and it is racy
across workers. Concurrent requests that observe an expired or missing start
time can each zero the counters, losing increments from completed requests;
recent failures just before the boundary are also discarded wholesale. Please
use bounded time buckets with atomic updates and add boundary-concurrency
coverage.
##########
apisix/plugins/api-breaker.lua:
##########
@@ -61,70 +65,200 @@ local schema = {
type = "integer",
minimum = 3,
default = 300,
+ description = "Circuit breaker duration in seconds " ..
+ "(applies to both count and ratio policies)"
},
- unhealthy = {
- type = "object",
- properties = {
- http_statuses = {
- type = "array",
- minItems = 1,
- items = {
+ policy = {
+ type = "string",
+ enum = { "unhealthy-count", "unhealthy-ratio" },
+ default = "unhealthy-count",
+ }
+ },
+ required = { "break_response_code" },
+ ["if"] = {
+ properties = {
+ policy = {
+ enum = { "unhealthy-count" },
+ },
+ },
+ },
+ ["then"] = {
+ properties = {
+ unhealthy = {
+ type = "object",
+ properties = {
+ http_statuses = {
+ type = "array",
+ minItems = 1,
+ items = {
+ type = "integer",
+ minimum = 500,
+ maximum = 599,
+ },
+ uniqueItems = true,
+ default = { 500 }
+ },
+ failures = {
type = "integer",
- minimum = 500,
- maximum = 599,
+ minimum = 1,
+ default = 3,
+ }
+ },
+ default = { http_statuses = { 500 }, failures = 3 }
+ },
+ healthy = {
+ type = "object",
+ properties = {
+ http_statuses = {
+ type = "array",
+ minItems = 1,
+ items = {
+ type = "integer",
+ minimum = 200,
+ maximum = 499,
+ },
+ uniqueItems = true,
+ default = { 200 }
},
- uniqueItems = true,
- default = {500}
+ successes = {
+ type = "integer",
+ minimum = 1,
+ default = 3,
+ }
+ },
+ default = { http_statuses = { 200 }, successes = 3 }
+ }
+ }
+ },
+ ["else"] = {
+ ["if"] = {
+ properties = {
+ policy = {
+ enum = { "unhealthy-ratio" },
},
- failures = {
- type = "integer",
- minimum = 1,
- default = 3,
- }
},
- default = {http_statuses = {500}, failures = 3}
},
- healthy = {
- type = "object",
+ ["then"] = {
properties = {
- http_statuses = {
- type = "array",
- minItems = 1,
- items = {
- type = "integer",
- minimum = 200,
- maximum = 499,
+ unhealthy = {
+ type = "object",
+ properties = {
+ http_statuses = {
+ type = "array",
+ minItems = 1,
+ items = {
+ type = "integer",
+ minimum = 500,
+ maximum = 599,
+ },
+ uniqueItems = true,
+ default = { 500 }
+ },
+ error_ratio = {
+ type = "number",
+ minimum = 0,
+ maximum = 1,
+ default = 0.5,
+ description = "Failure rate threshold to trigger
circuit breaker"
+ },
+ min_request_threshold = {
+ type = "integer",
+ minimum = 1,
+ default = 10,
+ description = "Minimum number of calls before " ..
+ "circuit breaker can be triggered"
+ },
+ sliding_window_size = {
+ type = "integer",
+ minimum = 10,
+ maximum = 3600,
+ default = 300,
+ description = "Size of the sliding window in
seconds"
+ },
+ half_open_max_calls = {
+ type = "integer",
+ minimum = 1,
+ maximum = 20,
+ default = 3,
+ description = "Number of permitted calls when " ..
+ "circuit breaker is half-open"
+ }
},
- uniqueItems = true,
- default = {200}
+ default = {
+ http_statuses = { 500 },
+ error_ratio = 0.5,
+ min_request_threshold = 10,
+ sliding_window_size = 300,
+ half_open_max_calls = 3
+ }
},
- successes = {
- type = "integer",
- minimum = 1,
- default = 3,
+ healthy = {
+ type = "object",
+ properties = {
+ http_statuses = {
+ type = "array",
+ minItems = 1,
+ items = {
+ type = "integer",
+ minimum = 200,
+ maximum = 499,
+ },
+ uniqueItems = true,
+ default = { 200 }
+ },
+ success_ratio = {
+ type = "number",
+ minimum = 0,
+ maximum = 1,
+ default = 0.6,
+ description = "Success rate threshold to close
circuit breaker " ..
+ "from half-open state"
+ }
+ },
+ default = { http_statuses = { 200 }, success_ratio = 0.6 }
}
- },
- default = {http_statuses = {200}, successes = 3}
+ }
}
- },
- required = {"break_response_code"},
+ }
}
-
+-- Key generation functions (based on latest APISIX version)
local function gen_healthy_key(ctx)
return "healthy-" .. core.request.get_host(ctx) .. ctx.var.uri
end
-
local function gen_unhealthy_key(ctx)
return "unhealthy-" .. core.request.get_host(ctx) .. ctx.var.uri
end
-
local function gen_lasttime_key(ctx)
return "unhealthy-lasttime" .. core.request.get_host(ctx) .. ctx.var.uri
end
+-- New key generation functions for ratio policy
+local function gen_state_key(ctx)
+ return "cb-state-" .. core.request.get_host(ctx) .. ctx.var.uri
+end
+
+local function gen_total_requests_key(ctx)
+ return "cb-total-" .. core.request.get_host(ctx) .. ctx.var.uri
Review Comment:
P1 — These statistics are keyed by the raw URI and have no normal TTL. On a
wildcard or high-cardinality route, every distinct path creates persistent
`cb-total`, `cb-window`, and failure entries, which can fill
`plugin-api-breaker` and evict unrelated breaker state. Please key by a stable
route/service configuration identity and expire or otherwise bound the buckets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
