SkyeYoung commented on code in PR #12448:
URL: https://github.com/apache/apisix/pull/12448#discussion_r2244528186
##########
docs/zh/latest/discovery/consul.md:
##########
Review Comment:
Weird.
Why are there so much content here that seem to be completely unrelated to
this PR feature.
Welcome to submit these extra good content individually through a new PR,
thx.
But please make sure that one PR focuses on doing only one thing as much as
possible.
##########
t/discovery/consul_dump.t:
##########
@@ -95,7 +95,7 @@ discovery:
--- request
GET /t
--- response_body
-{"service_a":[{"host":"127.0.0.1","port":30511,"weight":1}],"service_b":[{"host":"127.0.0.1","port":8002,"weight":1}]}
+{"service_a":[{"host":"127.0.0.1","metadata":{"service_a_version":"4.0"},"port":30511,"weight":1}],"service_b":[{"host":"127.0.0.1","metadata":{"service_b_version":"4.1"},"port":8002,"weight":1}]}
Review Comment:
Why does it affect this place? I don't think this test should be changed
##########
t/discovery/consul2.t:
##########
@@ -145,7 +145,7 @@ location /consul3 {
"PUT /consul2/deregister/service_a2",
"PUT /consul3/deregister/service_a1",
"PUT /consul3/deregister/service_a2",
- "PUT /consul1/register\n" .
"{\"ID\":\"service_a1\",\"Name\":\"service_a\",\"Tags\":[\"primary\",\"v1\"],\"Address\":\"127.0.0.1\",\"Port\":30511,\"Meta\":{\"service_a_version\":\"4.0\"},\"EnableTagOverride\":false,\"Weights\":{\"Passing\":10,\"Warning\":1}}",
+ "PUT /consul1/register\n" .
"{\"ID\":\"service_a1\",\"Name\":\"service_a\",\"Tags\":[\"primary\",\"v1\"],\"Address\":\"127.0.0.1\",\"Port\":30511,\"Meta\":{\"service_a_version\":\"4.0\",
\"weight\":\"100\"},\"EnableTagOverride\":false,\"Weights\":{\"Passing\":10,\"Warning\":1}}",
Review Comment:
Based on my questions regarding the changes related to `weight`, I think the
testing for this part needs to be reconsidered.
##########
apisix/utils/discovery.lua:
##########
@@ -0,0 +1,55 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local ipairs = ipairs
+local pairs = pairs
+
+local _M = {}
+
+local function do_metadata_match(inst, filters)
Review Comment:
What does "inst" mean?
Looking at the logic of how you use this function, I think "node" or
"upstream_node" would be better than the current meaning.
Regardless, please adjust the variable name or add some comments.
##########
apisix/utils/discovery.lua:
##########
@@ -0,0 +1,55 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local ipairs = ipairs
+local pairs = pairs
+
+local _M = {}
+
+local function do_metadata_match(inst, filters)
+ local metadata = inst.metadata or {}
Review Comment:
Should we return early if `inst.metadata` does not exist?
The current logic seems to still iterate once when `metadata={}`:
```lua
for _, value in ipairs(values) do
...
end
##########
apisix/discovery/consul/init.lua:
##########
@@ -511,11 +514,17 @@ function _M.connect(premature, consul_server, retry_delay)
local nodes = up_services[service_name]
local nodes_uniq = {}
for _, node in ipairs(result.body) do
- if not node.Service then
+ local service = node.Service
+ if not service then
goto CONTINUE
end
- local svc_address, svc_port = node.Service.Address,
node.Service.Port
+ local svc_address = service.Address
+ local svc_port = service.Port
+ local metadata = service.Meta
+ if type(metadata) ~= "table" then
+ metadata = nil
+ end
Review Comment:
Why do we need this logic? Pls add some comments.
##########
apisix/discovery/consul/init.lua:
##########
@@ -532,7 +541,8 @@ function _M.connect(premature, consul_server, retry_delay)
core.table.insert(nodes, {
host = svc_address,
port = tonumber(svc_port),
- weight = default_weight,
+ weight = metadata and tonumber(metadata.weight) or
default_weight,
Review Comment:
Should we get the weight from the metadata? Is this a common practice?
Should some comments be added?
--
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]
