-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
refactor(clustering): small improvements about incremental sync #13841
Conversation
26f0109
to
a7cd319
Compare
c572fc1
to
dfa3041
Compare
dfa3041
to
3057036
Compare
end | ||
|
||
else | ||
ngx_log(ngx_DEBUG, "notified ", node, " ", latest_version) |
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.
I think we still need the log for debugging. It will be hard to diagnose if we log nothing when succeeds
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.
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.
Let's remove it now, If we do need it in the practice we could add them back.
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.
@chronolaw With the level debug it's almost free in runtime. Any specific reason we are removing them?
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.
@StarlightIbuki we need to be careful with debug logs. Currently we don't have means of disabling just some debug logs or fine tune what is debug logged. While log messages like these may be beneficial when debugging incremental sync, it makes debugging everything else much harder as logs are just flooded with these. Logs that happen by background workers and timers or automated processes are especially problematic. And we already log errors. Unix: no news is good news
.
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.
Question.
kong/db/declarative/export.lua
Outdated
if not ok then | ||
return nil, err | ||
end | ||
local strategy = require("kong.clustering.services.sync.strategies.postgres").new(db) |
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.
Can we initialize this once and then just reuse, e.g. a module level get_latest_version
:
local strategy = require("kong.clustering.services.sync.strategies.postgres").new(db) | |
if not get_latest_version then | |
local strategy = require("kong.clustering.services.sync.strategies.postgres").new(db) | |
get_latest_version = function() | |
return strategy:get_latest_version() | |
end |
Or put it in some helper as it seems to be used in many places.
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.
It needs the global variable kong.db
, I think that it can not be a module level function.
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.
module level function that caches it on first use
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.
"kong.clustering.services.sync.strategies.postgres.new()
needs a db
param, if it is a module level function, kong.db
may not be initialized yet.
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.
@chronolaw I meant something like this:
local get_latest_version do
local strategy
local function get_latest_version_real()
return strategy:get_latest_version()
end
function get_latest_version()
strategy = require("kong.clustering.services.sync.strategies.postgres").new(kong.db)
get_latest_version = get_latest_version_real
return get_latest_version()
end
end
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.
done, many thanks.
Cherry-pick failed for Please cherry-pick the changes locally. git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13841-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13841-to-master-to-upstream
git checkout -b cherry-pick-13841-to-master-to-upstream
ancref=$(git merge-base 5f5f7d5ea51ac16263b32c1446a08e00816150f9 f6580d5fd317b6323f2db5f263c2410d8263ddc6)
git cherry-pick -x $ancref..f6580d5fd317b6323f2db5f263c2410d8263ddc6 |
Summary
KAG-5768
KAG-5776
KAG-5780
KAG-5788
Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
Fix #[issue number]