-
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
fix(clustering/sync): report last synced version #14194
Conversation
@@ -402,7 +402,7 @@ 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() -- one more call to report CP the latest version synced |
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.
Could you explain the reason? Why we should do this?
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.
This was the original behavior, where we rely on an extra call to tell the CP, that DP is on the latest version. Only with a "get_delta" call does CP gets the DP's current 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'm kind of impressed with the design, but not all of it, do you know where the documentation is?
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 that CP don't care about DP's version, what it will cause in CP?
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 UI needs this 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.
If we do need this, could we call get_delta
by notification?
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 That's a breaking change. Let's keep it untouched
I opened another similar PR for this, could you take a look? #14205 |
That should also work. I choose to do it like this because this is what we used to do. If you prefer notification over this, I'm okay with it. |
Summary
Checklist
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
KAG-6223