[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068199052


##
t/discovery/consul_dump.t:
##
@@ -75,9 +75,11 @@ discovery:
   consul:
 servers:
   - "http://127.0.0.1:8500";
+skip_services:
+  - "consul"
 dump:
   path: "consul.dump"
-  load_on_init: true
+  load_on_init: false

Review Comment:
   for the first time to start apisix, it will print some error log. Just for 
better reading log



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068221928


##
apisix/discovery/consul/init.lua:
##
@@ -237,53 +208,80 @@ function _M.connect(premature, consul_server, retry_delay)
 log.info("connect consul: ", consul_server.consul_server_url,
 ", watch_result status: ", watch_result.status,
 ", watch_result.headers.index: ", 
watch_result.headers['X-Consul-Index'],
-", consul_server.index: ", consul_server.index,
-", consul_server: ", json_delay_encode(consul_server, true))
+", consul_server.index: ", consul_server.index)
 
 -- if current index different last index then update service
 if consul_server.index ~= watch_result.headers['X-Consul-Index'] then
+local up_services = core.table.new(0, #watch_result.body)
+local consul_client_svc = resty_consul:new({
+host = consul_server.host,
+port = consul_server.port,
+connect_timeout = consul_server.connect_timeout,
+read_timeout = consul_server.read_timeout,

Review Comment:
   it is not needed, because `default_args` which has `index` arg, that will 
make **List Nodes for Service** got the wrong result.
   
   > [Consul API Blocking 
Queries](https://developer.hashicorp.com/consul/api-docs/features/blocking)



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068221928


##
apisix/discovery/consul/init.lua:
##
@@ -237,53 +208,80 @@ function _M.connect(premature, consul_server, retry_delay)
 log.info("connect consul: ", consul_server.consul_server_url,
 ", watch_result status: ", watch_result.status,
 ", watch_result.headers.index: ", 
watch_result.headers['X-Consul-Index'],
-", consul_server.index: ", consul_server.index,
-", consul_server: ", json_delay_encode(consul_server, true))
+", consul_server.index: ", consul_server.index)
 
 -- if current index different last index then update service
 if consul_server.index ~= watch_result.headers['X-Consul-Index'] then
+local up_services = core.table.new(0, #watch_result.body)
+local consul_client_svc = resty_consul:new({
+host = consul_server.host,
+port = consul_server.port,
+connect_timeout = consul_server.connect_timeout,
+read_timeout = consul_server.read_timeout,

Review Comment:
   it is not needed, because `default_args` which has `index` arg, that will 
make **[List Nodes for 
Service](https://developer.hashicorp.com/consul/api-docs/catalog#list-nodes-for-service)**
 got the wrong result.
   
   > [Consul API Blocking 
Queries](https://developer.hashicorp.com/consul/api-docs/features/blocking)



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068227529


##
apisix/discovery/consul/init.lua:
##
@@ -237,53 +208,80 @@ function _M.connect(premature, consul_server, retry_delay)
 log.info("connect consul: ", consul_server.consul_server_url,
 ", watch_result status: ", watch_result.status,
 ", watch_result.headers.index: ", 
watch_result.headers['X-Consul-Index'],
-", consul_server.index: ", consul_server.index,
-", consul_server: ", json_delay_encode(consul_server, true))
+", consul_server.index: ", consul_server.index)

Review Comment:
   Deleted by mistake, Undo it



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068229998


##
apisix/discovery/consul/init.lua:
##
@@ -237,53 +208,80 @@ function _M.connect(premature, consul_server, retry_delay)
 log.info("connect consul: ", consul_server.consul_server_url,
 ", watch_result status: ", watch_result.status,
 ", watch_result.headers.index: ", 
watch_result.headers['X-Consul-Index'],
-", consul_server.index: ", consul_server.index,
-", consul_server: ", json_delay_encode(consul_server, true))
+", consul_server.index: ", consul_server.index)
 
 -- if current index different last index then update service
 if consul_server.index ~= watch_result.headers['X-Consul-Index'] then
+local up_services = core.table.new(0, #watch_result.body)
+local consul_client_svc = resty_consul:new({
+host = consul_server.host,
+port = consul_server.port,
+connect_timeout = consul_server.connect_timeout,
+read_timeout = consul_server.read_timeout,
+})
+for service_name, _ in pairs(watch_result.body) do
+-- check is skip service

Review Comment:
   done



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068233846


##
t/discovery/consul_dump.t:
##
@@ -161,6 +165,8 @@ discovery:
   consul:
 servers:
   - "http://127.0.0.1:38500";
+skip_services:
+  - "consul"

Review Comment:
   the `consul` will register itself



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-12 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1068996136


##
t/discovery/consul_dump.t:
##
@@ -161,6 +165,8 @@ discovery:
   consul:
 servers:
   - "http://127.0.0.1:38500";
+skip_services:
+  - "consul"

Review Comment:
   Yes, consul will be registered by default. I dont think it would be better 
if we skip it by default. We can give users to choice whether to skip or not, 
that can be closed-loop control. Of course, we should mention this info in 
Consul doc



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-16 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1071035181


##
t/discovery/consul_dump.t:
##
@@ -161,6 +165,8 @@ discovery:
   consul:
 servers:
   - "http://127.0.0.1:38500";
+skip_services:
+  - "consul"

Review Comment:
   Does the following scenario exist?
   User A want to set up Consul route in APISIX with the Consul discovery plugin



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-16 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1071861277


##
t/discovery/consul_dump.t:
##
@@ -161,6 +165,8 @@ discovery:
   consul:
 servers:
   - "http://127.0.0.1:38500";
+skip_services:
+  - "consul"

Review Comment:
   OK, I add the `consul` as default skip service, and write it down in the doc



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [apisix] Fabriceli commented on a diff in pull request #8651: fix: fix fetch all service info from consul

2023-01-17 Thread GitBox 


Fabriceli commented on code in PR #8651:
URL: https://github.com/apache/apisix/pull/8651#discussion_r1073110176


##
apisix/discovery/consul/init.lua:
##
@@ -242,48 +215,76 @@ function _M.connect(premature, consul_server, retry_delay)
 
 -- if current index different last index then update service
 if consul_server.index ~= watch_result.headers['X-Consul-Index'] then
+local up_services = core.table.new(0, #watch_result.body)
+local consul_client_svc = resty_consul:new({
+host = consul_server.host,
+port = consul_server.port,
+connect_timeout = consul_server.connect_timeout,
+read_timeout = consul_server.read_timeout,
+})
+for service_name, _ in pairs(watch_result.body) do
+-- check if the service_name is 'skip service'
+if skip_service_map[service_name] then
+goto CONTINUE
+end
+-- get node from service
+local svc_url = consul_server.consul_sub_url .. "/" .. service_name
+local result, err = consul_client_svc:get(svc_url)
+local error_info = (err ~= nil and err) or
+((result ~= nil and result.status ~= 200) and 
result.status)
+if error_info then
+log.error("connect consul: ", consul_server.consul_server_url,
+" by svc url: ", svc_url, ", with error: ", error_info)

Review Comment:
   done



-- 
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-unsubscr...@apisix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org