Extending endpoint_addr in a compatible way, and adding a new configuration is not a good choice

Thank you for your reply.

What is your suggestion?
Can you give an example?
I refer to PR #7517

feat(clickhouse-logger): support multi clickhouse endpoints

Looking at this: https://github.com/apache/apisix/pull/7517/files#diff-d8d1670bb4d48884ee796f652172591cc371e5da972d7581ca585e60ddaab0f2R35, it also indicates the need to unify into one. You can refer to the definition of nodes_schema to allow endpoint_addrs to be set to multiple types: https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L289

Looking at this, it also indicates the need to unify into one. You can refer to the definition of nodes_schema to allow endpoint_addrs to be set to multiple types: https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L289

Thanks for the helpful advice!

Hi @soulbird.
I fixed the code according to your suggestion. please review it.

This should be put into the attribute table

Better to use endpoint_addrs with the new type.
Using different types in the same field brings us lots of trouble when we need to work with static-type language, like writing a Go client.

Hi @spacewander , thank you for your reply.
In my opinion, to use endpoint_addrs will cause trouble, this change is not backward compatible.

The code submitted for the first time is modified as follows:

 local schema = {
     type = "object",
     properties = {
+        -- deprecated, use "endpoint_addrs" instead
         endpoint_addr = {
             type = "string",
             pattern = "[^/]$",
         },
+        endpoint_addrs = {
+            type = "array",
+            minItems = 1,
+            items = {
+                type = "string",
+                pattern = "[^/]$",
+            },
+        },

I fixed the code according to @soulbird's suggestion.
#8604 (comment)

Keep endpoint_addr, add a new field endpoint_addrs to support both string and array better

this change is not backward compatible

I am confused. People can still use the old endpoint_addr configuration.

Keep endpoint_addr, add a new field endpoint_addrs to support both string and array better

It is not a good idea to have a field of multiple types. The upstream's nodes field is an example. People keep complaining that this field doesn't have a consistent type.

Understood. So we need to keep both endpoint_addr and endpoint_addrs, where endpoint_addrs is only specified as array type, is that right?

Yes

Let's add a deprecated mark like https://github.com/apache/apisix/blob/release/2.15/docs/en/latest/plugins/mqtt-proxy.md#attributes

We need to add a test to show that different endpoints will be chosen randomly.

I agree with you, but I don't know how.
Can you give an example?
I take PR #7517 as reference.
=== TEST 5: add plugin on routes using multi clickhouse-logger

What about hitting the path multiple times and triggering multiple sending?
Like https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md#send-request
Then we can check if all the endpoints are used via error log.

Hi @spacewander.
I fixed the code according to your suggestion. Please review it.

We can count the code like

Done.