[GitHub] [apisix] spacewander commented on a diff in pull request #8578: feat: limit count plugin support X-RateLimit-Reset

[GitBox <gi...@apache.org>]

spacewander commented on code in PR #8578:
URL: https://github.com/apache/apisix/pull/8578#discussion_r1058071416


##########
apisix/plugins/limit-count/init.lua:
##########
@@ -242,6 +245,15 @@ local function gen_limit_obj(conf, ctx)
     return core.lrucache.plugin_ctx(lrucache, ctx, extra_key, create_limit_obj, conf)
 end
 
+local function get_end_time(conf,key)
+    local create = function()
+        return {
+            endtime=0,
+        }
+    end
+
+    return resetlru(key,"",create,conf)

Review Comment:
   Don't forget to add space around the operator



##########
apisix/plugins/limit-count/init.lua:
##########
@@ -38,7 +39,9 @@ local lrucache = core.lrucache.new({
 local group_conf_lru = core.lrucache.new({
     type = 'plugin',
 })
-
+local resetlru = core.lrucache.new({

Review Comment:
   ```suggestion
   local reset_lru = core.lrucache.new({
   ```



##########
apisix/plugins/limit-count/init.lua:
##########
@@ -242,6 +245,15 @@ local function gen_limit_obj(conf, ctx)
     return core.lrucache.plugin_ctx(lrucache, ctx, extra_key, create_limit_obj, conf)
 end
 
+local function get_end_time(conf,key)
+    local create = function()

Review Comment:
   Please avoid creating new function per call



##########
docs/en/latest/plugins/limit-count.md:
##########
@@ -254,7 +254,7 @@ curl -i http://127.0.0.1:9180/apisix/admin/routes/1 \
 
 ## Example usage
 
-The above configuration limits to 2 requests in 60 seconds. The first two requests will work and the response headers will contain the headers `X-RateLimit-Limit` and `X-RateLimit-Remaining`:
+The above configuration limits to 2 requests in 60 seconds. The first two requests will work and the response headers will contain the headers `X-RateLimit-Limit` and `X-RateLimit-Remaining` and `X-RateLimit-Reset`:

Review Comment:
   Why the English version doesn't match the Chinese one?



##########
apisix/plugins/limit-count/init.lua:
##########
@@ -300,9 +327,28 @@ function _M.rate_limit(conf, ctx)
         return 500, {error_msg = "failed to limit count"}
     end
 
+    local reset
+    if remaining == conf.count - 1 then
+        -- set an end time
+        local end_time = ngx_time() + conf.time_window
+        -- save to lrucache by key
+        reset = conf.time_window
+        local end_time_obj = get_end_time(conf,key)

Review Comment:
   By default, the client IP will be used as key, which means there will be a great number of keys per conf. The size of lrucache is limited.
   
   There come two solutions:
   1. store the end time info in the actual backend like redis cluster.
   2. write a new cache with dynamical expire support to store the end time info, but this way is not memory-effective.
   
   Personally, I prefer solution 1.



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