refactor(clustering) dpcp config clean #8880
Conversation
chronolaw
force-pushed
the
refactor/dpcp_config_clean
branch
from
04b91ac
to
0155ea8
Compare
| if j % 100 == 0 then | ||
| i = 1 | ||
| o[i] = ngx_md5_bin(concat(o, ";", 1, 100)) | ||
| end |
There was a problem hiding this comment.
are there plans to fix this off-by-one error?
on every j==n*100, replaces all values in o with a hash of the first 100 and sets i to 1, so the next value (number 101) will go on o[2]. So when j==200, there will be 101 values in o (the last hash plus 100 values more) but it will again use only 100 values for the next hash, so it drops the hashes values of items number 201, 301, 401.... etc.
There was a problem hiding this comment.
It seems true. Shall we fix it?
There was a problem hiding this comment.
maybe we can use local i = 1 and new_tab(count < 100 and count +1 or 101, 0)
There was a problem hiding this comment.
Shall we fix it?
I don't know. mentioned it just that it can be discussed. probably in a separate PR.
There was a problem hiding this comment.
I think perhaps we can not change it because of the compatibility of old version.
There was a problem hiding this comment.
the hash method has changed several times already. there's no compatibility issue
There was a problem hiding this comment.
Let's fix the off-by-one error on this PR then then. Another PR will make everything take even longer.
There was a problem hiding this comment.
It will force us to modify all the hashes on the test, which is a pain.
New directive: Do it only if it can be done quickly on this PR. We have lived with the problem so far.
There was a problem hiding this comment.
I think we can do it later, now we should finish refactoring first.
| ["kong.clustering.version_negotiation"] = "kong/clustering/version_negotiation/init.lua", | ||
| ["kong.clustering.version_negotiation.services_known"] = "kong/clustering/version_negotiation/services_known.lua", | ||
| ["kong.clustering.version_negotiation.services_requested"] = "kong/clustering/version_negotiation/services_requested.lua", |
There was a problem hiding this comment.
when is this coming back?
There was a problem hiding this comment.
Without these, this PR is doing more than a refactor.
There was a problem hiding this comment.
After this PR, we will try another method to implement similar feature.
| if j % 100 == 0 then | ||
| i = 1 | ||
| o[i] = ngx_md5_bin(concat(o, ";", 1, 100)) | ||
| end |
There was a problem hiding this comment.
Let's fix the off-by-one error on this PR then then. Another PR will make everything take even longer.
| if hashes then | ||
| fill_empty_hashes(hashes) | ||
| end | ||
| local config_proto, msg = check_protocol_support(self.conf, self.cert, self.cert_key) |
There was a problem hiding this comment.
Can this be delayed until later? I mean: We leave it on init_worker for now in order to try to reach feature freeze, and we change it to something more robust as a bugfix, if we end up needing the resilience.
| if j % 100 == 0 then | ||
| i = 1 | ||
| o[i] = ngx_md5_bin(concat(o, ";", 1, 100)) | ||
| end |
There was a problem hiding this comment.
It will force us to modify all the hashes on the test, which is a pain.
New directive: Do it only if it can be done quickly on this PR. We have lived with the problem so far.
| ["kong.clustering.version_negotiation"] = "kong/clustering/version_negotiation/init.lua", | ||
| ["kong.clustering.version_negotiation.services_known"] = "kong/clustering/version_negotiation/services_known.lua", | ||
| ["kong.clustering.version_negotiation.services_requested"] = "kong/clustering/version_negotiation/services_requested.lua", |
There was a problem hiding this comment.
Without these, this PR is doing more than a refactor.
chronolaw
force-pushed
the
refactor/dpcp_config_clean
branch
from
b3f0436
to
7287710
Compare
### Summary DP/CP's code is too complex and not human readable, it is hard to understand the callback workflows. This PR separates the logic of update_config, moves the code into an isolated file, and also de-coupled dp/cp modules. This PR is same as #8880, but solved the merge conflict of #8888. ### Full changelog * separates the logic of `update_config` into `config_helper.lua` * do not use `self` to mimic oop * remove duplicated functions `extract_major_minor `and `validate_shared_cert` in wrpc_cp * add many functions in `utils.lua` * remove `version_handshake` * pick #8888's yield feature in `to_sorted_string` * update config hash test case
Summary
DP/CP's code is too complex and not human readable, it is hard to understand the callback workflows.
This PR separates the logic of update_config, moves the code into an isolated file,
and also de-coupled dp/cp modules.
Full changelog
update_configintoconfig_helper.luaselfto mimic oopextract_major_minorandvalidate_shared_certin wrpc_cputils.luaversion_handshake