Skip to content
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

feat: add consumer group #7980

Merged
merged 11 commits into from
Oct 11, 2022
Merged

Conversation

kingluo
Copy link
Contributor

@kingluo kingluo commented Sep 23, 2022

Description

add consumer group

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

apisix/core/ctx.lua Show resolved Hide resolved
conf.id = id

core.log.info("conf: ", core.json.delay_encode(conf))
local ok, err = core.schema.check(core.schema.plugin_config, conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't hijack the schema of plugin_config in other places.

end


function _M.put(id, conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a check in consumers. Let's make sure consumer.group_id points to an actual consumer group.

local err
consumer_groups, err = core.config.new("/consumer_groups", {
automatic = true,
item_schema = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we use schema to check the item?

$block->set_value("request", "GET /t");
}

if (!$block->no_error_log) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more common to use this style:

if ((!defined $block->error_log) && (!defined $block->no_error_log)) {

}
}
--- response_body
passed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. we need to verify that the plugin works well in the consumer_group
  2. we need to verify that consumer_group and consumer work well together

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like what other resources does, e.g. plugin_config, consumer, this test file is only for admin api test.
I would put plugin test and consumer stuff in t/node/consumer-group.t, and moreover, t/config-center-yaml/consumer-group.t, please keep in patient.

apisix/core/ctx.lua Show resolved Hide resolved
@@ -62,6 +63,12 @@ local function check_conf(username, conf)
end
end

if conf.group_id then
if consumer_group.get(conf.group_id) == nil then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For code style, I will recommend using the same solution in

if upstream_id then

  1. fetch the data from etcd if possible
  2. use a similar error message

spacewander
spacewander previously approved these changes Sep 27, 2022
tzssangglass
tzssangglass previously approved these changes Sep 27, 2022

When APISIX can't find the Consumer Group with the `group_id`, the Admin API is terminated with a status code of 400.

If a Consumer already has the `plugins` field configured, the plugins in the Consumer Group will effectively be merged to it. The same plugin in the Consumer Group will not override the ones configured directly in the Consumer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If a Consumer already has the `plugins` field configured, the plugins in the Consumer Group will effectively be merged to it. The same plugin in the Consumer Group will not override the ones configured directly in the Consumer.
If a Consumer already has the `plugins` field configured, the plugins in the Consumer Group will effectively be merged into it. The same plugin in the Consumer Group will not override the one configured directly in the Consumer.

instead of managing each consumer individually.

While configuring the same plugin for the same route, only one copy of the configuration is valid.
The order of precedence is `Consumer` > `Consumer Group` > `Route` > `plugin_config` > `Service`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to put the order to one place:

While configuring the same plugin, only one copy of the configuration is valid. The order of precedence is always `Consumer` > `Route` > `Plugin Config` > `Service`.


For example, if we configure a Consumer Group as shown below

```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add language mark like other examples

#
-->

Consumer Groups are used to extract commonly used [Plugin](./plugin.md) configurations and can be bound directly to a [Consumer](./consumer.md).
Copy link
Member

@spacewander spacewander Oct 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update the admin-api.md?

@@ -206,6 +206,7 @@ do
local apisix_var_names = {
balancer_ip = true,
balancer_port = true,
consumer_group_id = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


If a Consumer already has the `plugins` field configured, the plugins in the Consumer Group will effectively be merged to it. The same plugin in the Consumer Group will not override the ones configured directly in the Consumer.

For example, if we configure a Consumer Group as shown below
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add . or : in the end of sentence

}
```

to a Consumer as shown below,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to a Consumer as shown below,
To a Consumer as shown below.


to a Consumer as shown below,

```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add code language too

}'
```

When APISIX can't find the Consumer Group with the `group_id`, the Admin API is terminated with a status code of 400.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When APISIX can't find the Consumer Group with the `group_id`, the Admin API is terminated with a status code of 400.
When APISIX can't find the Consumer Group with the `group_id`, the Admin API is terminated with a status code of `400`.

@@ -38,6 +38,7 @@ additional variables.
| balancer_ip | core | The IP of picked upstream server. | 192.168.1.2 |
| balancer_port | core | The port of picked upstream server. | 80 |
| consumer_name | core | Username of Consumer. | |
| consumer_group_id | core | Group name of Consumer. | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| consumer_group_id | core | Group name of Consumer. | |
| consumer_group_id | core | Group ID of Consumer. | |

@@ -947,6 +948,35 @@ Sets Plugins which run globally. i.e these Plugins will be run before any Route/
| create_time | False | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |

## Consumer group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the Resources that support paging queries: list in this page

@@ -37,6 +37,7 @@ APISIX 除了支持 [NGINX 变量](http://nginx.org/en/docs/varindex.html)外,
| balancer_ip | core | 上游服务器的 IP 地址。 | 192.168.1.2 |
| balancer_port | core | 上游服务器的端口。 | 80 |
| consumer_name | core | 消费者的名称。 | |
| consumer_group_id | core | 消费者所在的组的名称。 | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's sync the change to the Chinese version too

@spacewander spacewander merged commit 7e2cc4f into apache:master Oct 11, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants