bzp2010 commented on code in PR #13066:
URL: https://github.com/apache/apisix/pull/13066#discussion_r2916041474


##########
apisix/discovery/consul/init.lua:
##########
@@ -66,53 +73,85 @@ local _M = {
 }
 
 
-local function discovery_consul_callback(data, event, source, pid)
-    all_services = data
-    log.notice("update local variable all_services, event is: ", event,
-        "source: ", source, "server pid:", pid,
-        ", all services: ", json_delay_encode(all_services, true))
-end
-
-
 function _M.all_nodes()
-    return all_services
+    local keys = consul_dict:get_keys(0)
+    local services = core.table.new(0, #keys)
+    for i, key in ipairs(keys) do
+        local value = consul_dict:get(key)
+        if value then
+            local nodes, err = core.json.decode(value)
+            if nodes then
+                services[key] = nodes
+            else
+                log.error("failed to decode nodes for service: ", key, ", 
error: ", err)
+            end
+        end
+
+        if i % 100 == 0 then
+            ngx.sleep(0)
+        end
+    end
+    return services
 end
 
 
 function _M.nodes(service_name)
-    if not all_services then
-        log.error("all_services is nil, failed to fetch nodes for : ", 
service_name)
-        return
+    local value = consul_dict:get(service_name)
+    if not value then

Review Comment:
   Each request using service discovery will directly retrieve from shdict, 
which I suspect may cause lock contention.
   Why not adopt the following pattern:
   
   1. Retrieve the node for the service name from lrucache. If present, it will 
return quickly without contention.
   2. If absent from lrucache, retrieve authoritative data prepared by 
privileged processes via the initialization function from shdict. Lrucache will 
then cache and return these.
   3. If the shdict also doesn't contain it, it's best to log the "not found" 
result in lrucache to prevent an unexpected service name from consistently 
bypassing the fast path cache. It will be cached via `neg_ttl`, 
https://github.com/apache/apisix/blob/master/apisix/core/lrucache.lua#L130.
   
   This ensures most requests complete on the fast path without accessing 
shdict. Only the next cache hit after a miss will fetch from shdict.
   The way currently being adopted is exactly the opposite.



-- 
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]

Reply via email to