Updating kafka logger to use the batch processor util #1358
Conversation
|
Hi @sshniro I think Kafka logger does not needs to use the batch processor since it already has the capability to push to the topic in an asynchronous way. Lua resty Kafka already has the feature already inbuilt I already mentioned that in the documentation file of the logger The message will write to the buffer first. It will send to the Kafka server when the buffer exceeds the batch_num or every flush_time flush the buffer in Async mode cc :- @membphis wdyt ? |
|
@Akayeshmantha I agree with you. The current way is simpler and enough. |
|
Closing the PR as agreed to use the builtin buffer of the library. |
|
Reopening PR due to the max number of timers which could be executed within a period |
|
@membphis the following PR is ready for review. |
|
@membphis please take a look |
| return | ||
| end | ||
|
|
||
| buffers[entry.route_id] = log_buffer |
There was a problem hiding this comment.
I have one question:
If there are no more new logs to be sent later, how to release the log_buffer object?
If the log_buffer is useless, how to delete it from buffers? If we don't delete it, it will make the buffers bigger.
we need to control the max count number of timer. the default value is 1024 for each worker process.
https://github.com/openresty/lua-nginx-module#lua_max_pending_timers
There was a problem hiding this comment.
Yes, @membphis I was also thinking about this problem,
For the memory problem:
we need to write a timer function which executes in all active loggers and finds stale buffer objects and remove the variable. Maybe it can run every 30 mins?
For the timer problem:
The buffer by default has an inactive timeout. If we don't send any logs for 30 seconds, by default it will send the existing logs and will not create any additional timers. Due to this fact, we won't be creating timers for inactive buffers. Only active buffers will have a timer running.
If we agree on the memory optimization technique, then I can send the fix in another PR.
A practical bottleneck is if a user configures like 1024 routes with a logger and actively using all the routes.
But I assume its not a practical scenario. To tackle this scenario we might have to have a global logger instead of a route based logger.
There was a problem hiding this comment.
@membphis any update on this?
|
A PR has been created to remove stale objects from the buffer table |
|
We are now creating a batch processor per route id: Just to reduce the number of keys getting created how about we create only one batch processor, if the host port and timeout values are the same? @membphis Because in reality, people will only have a single external log management platform. So when a request comes: local key = con.host .. conf.port .. conf.timeout .. conf.retry
local buffer = buffers[key] |
* apisix/master: change: limit the maximum length of Lua code to 100. (apache#1525) doc: fixed wrong configurations in the logger docs (apache#1530) feature: add batch request plugin. (apache#1388) plugin(kafka-logger): Updating kafka logger to use the batch processor util (apache#1358) test: reindex by tools `reindex`. (apache#1519) fix: skip tombstone mark when iterating the global values (apache#1517) bugfix: init `clean_handlers` when add new item from etcd. (apache#1412) doc: fix the doc style for serverless*.md (apache#1511) test: check lua code style in all Lua file under apisix/ (apache#1518) feat: support saving k8s deployment info to upstream (apache#1502) doc: update steps of build dashboard. (apache#1506) rocks: used tag instead of branch. (apache#1503) test: fix regex usage in some cases (apache#1504) doc: add short introduction about how to change log level (apache#1484) bugfix(lrucache): when creating cached objects, use resty-lock to avoid repeated creation. (apache#1486) bug: fixed wrong string join in limit-count plugin. (apache#1487) doc(readme.md): upload Node.js version for building dashboard. (apache#1485) release: released 1.2 version. (apache#1436) # Conflicts: # bin/apisix # t/node/upstream.t
Summary
Updating kafka logger to use the batch processor util.
Depends on #1349