-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: add batch request plugin. #1388
Conversation
@ShiningRush missing test case with invalid arguments |
and please take a look at the output of Travis: https://travis-ci.org/github/apache/incubator-apisix/jobs/670393219#L817 |
doc/plugins/api-aggregate-cn.md
Outdated
"admin-jwt":"xxxx" | ||
}, | ||
"timeout": 500, | ||
"pipeline": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad indentation
doc/plugins/api-aggregate-cn.md
Outdated
本插件默认启用。 | ||
|
||
## 聚合接口请求/响应 | ||
插件会为 `apisix` 创建一个 `/apisix/aggregate` 的聚合接口来处理你的聚合请求。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this plugin is using admin API to aggregate apis, why not use plugin's configures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, is here example about how to create a api via plugin's configures?API-Aggregation need a API to handle request.
We have discussed the the feature's detail, then rename the plugin to "batch-requests", it likes the batch APIof google and microsoft. |
apisix/plugins/batch-requests.lua
Outdated
|
||
local data, err = core.json.decode(req_body) | ||
if not data then | ||
core.response.exit(400, {message = "invalid request body", req_body = req_body, err = err}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use only one style, eg: {error_msg = "**** " .. err}
, it is easier for the client shows the error message.
here is the example: https://github.com/apache/incubator-apisix/pull/1388/files#diff-7a39e1834cde089a2da404821d733a92R111
please fix the other similar points.
apisix/plugins/batch-requests.lua
Outdated
httpc:set_timeout(data.timeout) | ||
local ok, err = httpc:connect("127.0.0.1", ngx.var.server_port) | ||
if not ok then | ||
core.response.exit(500, {message = "connect to apisix failed", err = err}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditoo
apisix/plugins/batch-requests.lua
Outdated
set_common_query(data) | ||
local responses, err = httpc:request_pipeline(data.pipeline) | ||
if not responses then | ||
core.response.exit(400, {message = "request failed", err = err}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditoo
apisix/plugins/batch-requests.lua
Outdated
end | ||
|
||
local aggregated_resp = {} | ||
for i,r in ipairs(responses) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this style is better: for _, res in ipairs(responses) do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res
may mislead to resource
, I changed to for _, resp in ipairs(responses) do
merged, many thx @ShiningRush |
* 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
support api aggregation mentioned in #603
(BTW: I will write doc for it when code review finish.)