You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/09/24 04:54:10 UTC
[GitHub] [apisix] Yiyiyimu opened a new pull request #2299: [WIP] fix: use etcd:watchcancel to reduce connections in etcdv3
Yiyiyimu opened a new pull request #2299:
URL: https://github.com/apache/apisix/pull/2299
### What this PR does / why we need it:
fix #2295
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495424405
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
@membphis I think we don't need to consider compatibility with v2
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#issuecomment-699839248
Hi @membphis is there something else we should do before merging
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#issuecomment-699873899
ping @moonming it seems good to go
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495410918
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
Because etcd v3 does not have 1000 event history limit anymore, it could support unlimited history.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495421739
##########
File path: apisix/core/config_etcd.lua
##########
@@ -123,6 +124,11 @@ local function waitdir(etcd_cli, key, modified_index, timeout)
res, err = res_func()
end
+ if http_cli then
+ etcd_cli:watchcancel(http_cli)
Review comment:
thx, test added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2299:
URL: https://github.com/apache/apisix/pull/2299
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495943254
##########
File path: apisix/core/config_etcd.lua
##########
@@ -123,6 +124,15 @@ local function waitdir(etcd_cli, key, modified_index, timeout)
res, err = res_func()
end
+ if http_cli then
+ local res_cancel, err_cancel = etcd_cli:watchcancel(http_cli)
+ if res_cancel and res_cancel == 1 then
Review comment:
Thx! Should I create a PR for it or leave it to the next PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495945060
##########
File path: apisix/core/config_etcd.lua
##########
@@ -123,6 +124,15 @@ local function waitdir(etcd_cli, key, modified_index, timeout)
res, err = res_func()
end
+ if http_cli then
+ local res_cancel, err_cancel = etcd_cli:watchcancel(http_cli)
+ if res_cancel and res_cancel == 1 then
Review comment:
sure,welcome
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495827965
##########
File path: apisix/core/config_etcd.lua
##########
@@ -123,6 +124,15 @@ local function waitdir(etcd_cli, key, modified_index, timeout)
res, err = res_func()
end
+ if http_cli then
+ local res_cancel, err_cancel = etcd_cli:watchcancel(http_cli)
+ if res_cancel and res_cancel == 1 then
Review comment:
`if res_cancel == 1 then` is enough
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495820272
##########
File path: apisix/core/etcd.lua
##########
@@ -36,6 +36,7 @@ local function new()
etcd_conf.host = nil
etcd_conf.prefix = nil
etcd_conf.protocol = "v3"
+ etcd_conf.api_prefix = "/v3"
Review comment:
In lua-resty-etcd. If api_prefix is not set, it would check the version of etcd to find the correct api_prefix to use in uri. Since we support etcd v3.4+ only, we could directly set it to avoid useless version check each time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495808697
##########
File path: apisix/core/etcd.lua
##########
@@ -36,6 +36,7 @@ local function new()
etcd_conf.host = nil
etcd_conf.prefix = nil
etcd_conf.protocol = "v3"
+ etcd_conf.api_prefix = "/v3"
Review comment:
Where will it be used?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495411846
##########
File path: apisix/core/config_etcd.lua
##########
@@ -181,10 +181,6 @@ local function sync_data(self)
return false, err
end
- if not dir_res.dir then
- return false, self.key .. " is not a dir"
- end
-
Review comment:
Currently I still set `dir = true` in [etcd protocol conversion](https://github.com/apache/apisix/blob/9ac02f66b14fa3c1b5a8f9341a82b664ceda17fb/apisix/core/etcd.lua#L61) for consistency between protocols. But I noticed there is only one test case ([global.t TEST 3](https://github.com/apache/apisix/blob/9ac02f66b14fa3c1b5a8f9341a82b664ceda17fb/t/admin/global-rules.t#L125-L127)) needs this parameter, so maybe we could remove it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495412704
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
But we also need to consider history compaction, or the accumulation of history would affect performance in a long term. Reference: https://etcd.io/docs/v3.4.0/op-guide/maintenance/#history-compaction
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495457990
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
got it. many thanks for explain
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495361243
##########
File path: apisix/core/config_etcd.lua
##########
@@ -123,6 +124,11 @@ local function waitdir(etcd_cli, key, modified_index, timeout)
res, err = res_func()
end
+ if http_cli then
+ etcd_cli:watchcancel(http_cli)
Review comment:
need test case.
we can use this way: write a log and confirm it in test case
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
why we delete this code block? it is useful for etcd `v2`. please confirm this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2299: bugfix: use etcd:watchcancel to reduce connections in etcdv3
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2299:
URL: https://github.com/apache/apisix/pull/2299#discussion_r495412704
##########
File path: apisix/core/config_etcd.lua
##########
@@ -246,24 +248,9 @@ local function sync_data(self)
return true
end
- -- for fetch the etcd index
- local key_res, _ = getkey(self.etcd_cli, self.key)
-
local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
-
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if err == "timeout" then
Review comment:
But we also need to consider history compaction, or the accumulation of history would affect performance in a long term. Reference: https://etcd.io/docs/v3.4.0/op-guide/maintenance/#history-compaction
There are two options to tackle this problem:
1. Use etcd native client, which shown on the website, that could auto compact history at certain revision/period gap. But since etcd and apisix could be deployed on different machines so this could not work.
2. Develop `compact` in lua-resty-etcd. I came across some problem on it three months ago. I could go back trying to fix it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org