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.

Sign up for GitHub

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

Jump to bottom

refactor(clustering/sync): improve readability and debugability #14170

Closed
wants to merge 1 commit into from
Closed

refactor(clustering/sync): improve readability and debugability #14170

wants to merge 1 commit into from

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Jan 15, 2025 •
edited
Loading

Summary

  1. abstract functions and reshape the code, including error handling
  2. extra call to notify CP the state of sync;
  3. more debuging logs;
  4. sync once retry will not count timeout as retry, allowing DP to sync deltas right after a full sync

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

KAG-6177

@github-actions github-actions bot added core/clustering cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Jan 15, 2025
@StarlightIbuki StarlightIbuki removed the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 15, 2025
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 15, 2025
@chronolaw chronolaw changed the title refactor(sync.v2): improve readability and debugability refactor(clustering/sync): improve readability and debugability Jan 15, 2025
chronolaw
chronolaw reviewed Jan 15, 2025
View reviewed changes
kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
kong/clustering/services/sync/rpc.lua Show resolved Hide resolved
kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
kong/clustering/services/sync/rpc.lua Outdated Show resolved Hide resolved
kong/clustering/services/sync/rpc.lua Show resolved Hide resolved
@StarlightIbuki StarlightIbuki force-pushed the refactor/rpc-sync-ce branch 3 times, most recently from 537d6f2 to b18175b Compare January 15, 2025 09:45
@StarlightIbuki StarlightIbuki force-pushed the refactor/rpc-sync-ce branch 4 times, most recently from ee07453 to 43975e3 Compare January 16, 2025 06:37
chobits
chobits approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@chobits chobits left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nice!! all my comments are included in its associated EE pr, which have been resolved by xuming already

@StarlightIbuki StarlightIbuki force-pushed the refactor/rpc-sync-ce branch 9 times, most recently from d9940f1 to 2ecd057 Compare January 17, 2025 09:04
@StarlightIbuki StarlightIbuki force-pushed the refactor/rpc-sync-ce branch 2 times, most recently from 354b255 to 2918b81 Compare January 17, 2025 09:07
@StarlightIbuki
refactor(sync.v2): improve readability and debugability
8ae2931 
1. abstract functions and reshape the code, including error handling
2. extra call to notify CP the state of sync;
3. more debuging logs;
4. sync once retry will not count timeout as retry, allowing DP to sync deltas right after a full sync

KAG-6177
chronolaw
chronolaw reviewed Jan 20, 2025
View reviewed changes
kong/clustering/services/sync/rpc.lua
end


local function lmdb_delete_impl(db, t, typ, opts, delete_if_exist, old_entity, find_entity_err)
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 that it is premature optimization, the original code is easy to understand.

kong/clustering/services/sync/rpc.lua
ngx_log(ngx_ERR, err)

if is_full_sync then
return do_sync() -- retry without releasing the mutex as the DP has nothing to do except syncing
Copy link
Contributor

Choose a reason for hiding this comment

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

It changed the original behavior, could we do it in another PR?

kong/clustering/services/sync/rpc.lua
t:db_drop(false)
end

local db = kong.db

local version = ""
local version = current_version
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between this and ""? and improvement?

kong/clustering/services/sync/rpc.lua
if not ok then
return nil, err
end
return events.declarative_reconfigure_notify(reconfigure_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very useful refactor, it don't give us more improvement.

kong/clustering/services/sync/rpc.lua
@@ -400,17 +433,27 @@ function sync_once_impl(premature, retry_count)
local current_version = get_current_version()
if current_version >= latest_notified_version then
ngx_log(ngx_DEBUG, "version already updated")
return
return sync_handler() -- call get_delta once more to report to the CP that we are up-to-date
Copy link
Contributor

Choose a reason for hiding this comment

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

It changed the original behavior, could we do it in another PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd agree. The refactor PR should only contain refactor and other behavior changes should be put into a separate PR. This way it will makes the reviewers lives easier and also make it easier to track issues in the future.

kong/clustering/services/sync/rpc.lua
Comment on lines +442 to +444
if err ~= "timeout" then
retry_count = retry_count + 1
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to the tests in #14174 to ensure it will not break the cases.

kong/clustering/services/sync/rpc.lua
-- in some cases, the new spawned timer will be switched to immediately,
-- preventing the coroutine who possesses the mutex to run
-- to let other coroutines has a chance to run
yield()
Copy link
Contributor

Choose a reason for hiding this comment

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

It changed the original behavior, could we do it in another PR?

@chronolaw
Copy link
Contributor

I suggest that this PR should be a PURE refactor PR, not introduce any feature or change.

chronolaw
chronolaw reviewed Jan 20, 2025
View reviewed changes
kong/clustering/services/sync/rpc.lua
end

-- ns_deltas should look like:
-- { default = { deltas = { ... }, wipe = true, }, }
-- { default = { deltas = { ... } }, }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the comments?

@chobits chobits marked this pull request as draft January 21, 2025 07:22
@StarlightIbuki StarlightIbuki deleted the refactor/rpc-sync-ce branch January 21, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chronolaw chronolaw chronolaw left review comments

@AndyZhang0707 AndyZhang0707 AndyZhang0707 left review comments

@chobits chobits chobits approved these changes

Assignees

@StarlightIbuki StarlightIbuki

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/clustering size/L skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StarlightIbuki @chronolaw @chobits @AndyZhang0707