You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "leslie-tsang (via GitHub)" <gi...@apache.org> on 2023/05/03 12:55:09 UTC
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9204: fix(consul): support to fetch only health endpoint
leslie-tsang commented on code in PR #9204:
URL: https://github.com/apache/apisix/pull/9204#discussion_r1183615907
##########
apisix/discovery/consul/init.lua:
##########
@@ -176,80 +190,304 @@ local function get_retry_delay(retry_delay)
end
+local function get_opts(consul_server, is_catalog)
+ local opts = {
+ host = consul_server.host,
+ port = consul_server.port,
+ connect_timeout = consul_server.connect_timeout,
+ read_timeout = consul_server.read_timeout,
+ }
+ if consul_server.keepalive then
+ if is_catalog then
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.catalog_index,
+ }
+ else
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.health_index,
+ }
+ end
+ end
+
+ return opts
Review Comment:
```suggestion
if not consul_server.keepalive then
return opts
end
if is_catalog then
opts.default_args = {
wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
index = consul_server.catalog_index,
}
else
opts.default_args = {
wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
index = consul_server.health_index,
}
end
return opts
```
Would be better ?
##########
apisix/discovery/consul/init.lua:
##########
@@ -176,80 +190,304 @@ local function get_retry_delay(retry_delay)
end
+local function get_opts(consul_server, is_catalog)
+ local opts = {
+ host = consul_server.host,
+ port = consul_server.port,
+ connect_timeout = consul_server.connect_timeout,
+ read_timeout = consul_server.read_timeout,
+ }
+ if consul_server.keepalive then
+ if is_catalog then
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.catalog_index,
+ }
+ else
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.health_index,
+ }
+ end
+ end
+
+ return opts
+end
+
+
+local function watch_catalog(consul_server)
+ local client = resty_consul:new(get_opts(consul_server, true))
+
+ ::RETRY::
+ local watch_result, watch_err = client:get(consul_server.consul_watch_catalog_url)
+ local watch_error_info = (watch_err ~= nil and watch_err)
+ or ((watch_result ~= nil and watch_result.status ~= 200)
+ and watch_result.status)
Review Comment:
```suggestion
local watch_error_info = (watch_err ~= nil and watch_err)
or ((watch_result ~= nil and watch_result.status ~= 200)
and watch_result.status)
```
Plz follow the [CODE_STYLE](https://github.com/apache/apisix/blob/master/CODE_STYLE.md#maximum-length-per-line)
##########
apisix/discovery/consul/init.lua:
##########
@@ -176,80 +190,304 @@ local function get_retry_delay(retry_delay)
end
+local function get_opts(consul_server, is_catalog)
+ local opts = {
+ host = consul_server.host,
+ port = consul_server.port,
+ connect_timeout = consul_server.connect_timeout,
+ read_timeout = consul_server.read_timeout,
+ }
+ if consul_server.keepalive then
+ if is_catalog then
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.catalog_index,
+ }
+ else
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.health_index,
+ }
+ end
+ end
+
+ return opts
+end
+
+
+local function watch_catalog(consul_server)
+ local client = resty_consul:new(get_opts(consul_server, true))
+
+ ::RETRY::
+ local watch_result, watch_err = client:get(consul_server.consul_watch_catalog_url)
+ local watch_error_info = (watch_err ~= nil and watch_err)
+ or ((watch_result ~= nil and watch_result.status ~= 200)
+ and watch_result.status)
+ if watch_error_info then
+ log.error("connect consul: ", consul_server.consul_server_url,
+ " by sub url: ", consul_server.consul_watch_catalog_url,
+ ", got watch result: ", json_delay_encode(watch_result, true),
+ ", with error: ", watch_error_info)
+
+ return watch_type_catalog, default_catalog_error_index
+ end
+
+ if consul_server.catalog_index > 0
+ and consul_server.catalog_index == tonumber(watch_result.headers['X-Consul-Index']) then
+ local random_delay = math_random(default_random_seed)
+ log.info("watch catalog has no change, re-watch consul after ", random_delay, " seconds")
+ core_sleep(random_delay)
+ goto RETRY
+ end
+
+ return watch_type_catalog, watch_result.headers['X-Consul-Index']
+end
+
+
+local function watch_health(consul_server)
+ local client = resty_consul:new(get_opts(consul_server, false))
+
+ ::RETRY::
+ local watch_result, watch_err = client:get(consul_server.consul_watch_health_url)
+ local watch_error_info = (watch_err ~= nil and watch_err)
+ or ((watch_result ~= nil and watch_result.status ~= 200)
+ and watch_result.status)
+ if watch_error_info then
+ log.error("connect consul: ", consul_server.consul_server_url,
+ " by sub url: ", consul_server.consul_watch_health_url,
+ ", got watch result: ", json_delay_encode(watch_result, true),
+ ", with error: ", watch_error_info)
+
+ return watch_type_health, default_health_error_index
+ end
+
+ if consul_server.health_index > 0
+ and consul_server.health_index == tonumber(watch_result.headers['X-Consul-Index']) then
+ local random_delay = math_random(default_random_seed)
+ log.info("watch health has no change, re-watch consul after ", random_delay, " seconds")
+ core_sleep(random_delay)
+ goto RETRY
+ end
+
+ return watch_type_health, watch_result.headers['X-Consul-Index']
+end
+
+
+local function check_keepalive(consul_server, retry_delay)
+ if consul_server.keepalive then
+ local ok, err = ngx_timer_at(0, _M.connect, consul_server, retry_delay)
+ if not ok then
+ log.error("create ngx_timer_at got error: ", err)
+ return
+ end
+ end
+end
+
+
+local function update_index(consul_server, catalog_index, health_index)
+ local c_index = 0
+ local h_index = 0
+ if catalog_index ~= nil then
+ c_index = tonumber(catalog_index)
+ end
+
+ if health_index ~= nil then
+ h_index = tonumber(health_index)
+ end
+
+ if c_index > 0 then
+ consul_server.catalog_index = c_index
+ end
+
+ if h_index > 0 then
+ consul_server.health_index = h_index
+ end
+end
+
+
+local function is_not_empty(value)
+ if value == nil or value == null
+ or (type(value) == "table" and not next(value))
+ or (type(value) == "string" and value == "") then
+ return false
Review Comment:
```suggestion
if value == nil or value == null
or (type(value) == "table" and not next(value))
or (type(value) == "string" and value == "")
then
return false
```
Make it more easier to read. :)
##########
apisix/discovery/consul/init.lua:
##########
@@ -176,80 +190,304 @@ local function get_retry_delay(retry_delay)
end
+local function get_opts(consul_server, is_catalog)
+ local opts = {
+ host = consul_server.host,
+ port = consul_server.port,
+ connect_timeout = consul_server.connect_timeout,
+ read_timeout = consul_server.read_timeout,
+ }
+ if consul_server.keepalive then
+ if is_catalog then
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.catalog_index,
+ }
+ else
+ opts.default_args = {
+ wait = consul_server.wait_timeout, --blocked wait!=0; unblocked by wait=0
+ index = consul_server.health_index,
+ }
+ end
+ end
+
+ return opts
+end
+
+
+local function watch_catalog(consul_server)
+ local client = resty_consul:new(get_opts(consul_server, true))
+
+ ::RETRY::
+ local watch_result, watch_err = client:get(consul_server.consul_watch_catalog_url)
+ local watch_error_info = (watch_err ~= nil and watch_err)
+ or ((watch_result ~= nil and watch_result.status ~= 200)
+ and watch_result.status)
+ if watch_error_info then
+ log.error("connect consul: ", consul_server.consul_server_url,
+ " by sub url: ", consul_server.consul_watch_catalog_url,
+ ", got watch result: ", json_delay_encode(watch_result, true),
+ ", with error: ", watch_error_info)
+
+ return watch_type_catalog, default_catalog_error_index
+ end
+
+ if consul_server.catalog_index > 0
+ and consul_server.catalog_index == tonumber(watch_result.headers['X-Consul-Index']) then
+ local random_delay = math_random(default_random_seed)
+ log.info("watch catalog has no change, re-watch consul after ", random_delay, " seconds")
+ core_sleep(random_delay)
+ goto RETRY
+ end
+
+ return watch_type_catalog, watch_result.headers['X-Consul-Index']
+end
+
+
+local function watch_health(consul_server)
+ local client = resty_consul:new(get_opts(consul_server, false))
+
+ ::RETRY::
+ local watch_result, watch_err = client:get(consul_server.consul_watch_health_url)
+ local watch_error_info = (watch_err ~= nil and watch_err)
+ or ((watch_result ~= nil and watch_result.status ~= 200)
+ and watch_result.status)
+ if watch_error_info then
+ log.error("connect consul: ", consul_server.consul_server_url,
+ " by sub url: ", consul_server.consul_watch_health_url,
+ ", got watch result: ", json_delay_encode(watch_result, true),
Review Comment:
```suggestion
", got watch result: ", json_delay_encode(watch_result, true),
```
No need for `json_delay_encode` as the log level is `error` by default ?
##########
apisix/discovery/consul/init.lua:
##########
@@ -44,9 +52,15 @@ local events_list
local consul_services
local default_skip_services = {"consul"}
+local default_random_seed = 5
Review Comment:
Would a new variable name be better?
It confuses me as a `seed` as it don't do the seed stuff, would be better to like `range` or something else ?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org