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

refactor(clustering): small improvements about incremental sync #13841

Merged
merged 15 commits into from
Nov 11, 2024

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Nov 6, 2024

Summary

KAG-5768
KAG-5776
KAG-5780
KAG-5788

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@github-actions github-actions bot added core/clustering cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Nov 6, 2024
@chronolaw chronolaw changed the title refactor(dbless): small improvements about incremental sync refactor(clustering): small improvements about incremental sync Nov 7, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 7, 2024
@chronolaw chronolaw force-pushed the refactor/improve_incremental_sync branch 2 times, most recently from 26f0109 to a7cd319 Compare November 7, 2024 22:38
@chronolaw chronolaw force-pushed the refactor/improve_incremental_sync branch from c572fc1 to dfa3041 Compare November 8, 2024 00:05
@chronolaw chronolaw force-pushed the refactor/improve_incremental_sync branch from dfa3041 to 3057036 Compare November 8, 2024 02:02
@chronolaw chronolaw marked this pull request as ready for review November 8, 2024 04:01
end

else
ngx_log(ngx_DEBUG, "notified ", node, " ", latest_version)
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Member

@bungle bungle Nov 8, 2024

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.

kong/clustering/services/sync/rpc.lua Show resolved Hide resolved
bungle

This comment was marked as outdated.

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

Question.

kong/clustering/services/sync/rpc.lua Show resolved Hide resolved
if not ok then
return nil, err
end
local strategy = require("kong.clustering.services.sync.strategies.postgres").new(db)
Copy link
Member

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:

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

@chronolaw chronolaw Nov 11, 2024

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.

Copy link
Member

@bungle bungle Nov 11, 2024

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, many thanks.

@bungle bungle merged commit 1fd3a76 into master Nov 11, 2024
28 checks passed
@bungle bungle deleted the refactor/improve_incremental_sync branch November 11, 2024 15:46
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

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

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Nov 11, 2024
@kikito kikito removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants