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/rpc): tunings and fixes for rubustness #13771

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Oct 17, 2024

Summary

KAG-5617

Related tickets:
KAG-5602

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/db cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Oct 17, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 22, 2024
@chronolaw chronolaw changed the title refactor(clustering/rpc): tunings for rubustness refactor(clustering/rpc): tunings and fixes for rubustness Oct 22, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 24, 2024
@chronolaw chronolaw marked this pull request as ready for review October 29, 2024 11:57
@team-gateway-bot team-gateway-bot added the author/community PRs from the open-source community (not Kong Inc) label Oct 29, 2024

-- If the item belongs to a specific workspace,
-- use it directly without using the default one.
local ws_id = item.ws_id or workspace_id(schema, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: need to add a test to check the ws_id value if the schema is non-workspaceable. https://konghq.atlassian.net/browse/KAG-5706

This comment is not a block.

Comment on lines 318 to 322
for _, wid in ipairs {ws_id, GLOBAL_WORKSPACE_TAG} do
local key = unique_field_key(entity_name, wid, "cache_key", cache_key)

-- store item_key or nil into lmdb
t:set(key, idx_value)
Copy link
Contributor

@chobits chobits Oct 30, 2024

Choose a reason for hiding this comment

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

not a blocker, just a comment for other reviewers.

TODO: we only want to save the entity with key containing specific ws_id, not a GLOBAL workspace id like*, which does not consume too much memory space for lmdb. https://konghq.atlassian.net/browse/KAG-5704

Comment on lines 390 to 395
-- * <entity_name>|*|*|<pk_string> => serialized item
-- * <entity_name>|<ws_id>|*|<pk_string> => actual item key
--
-- * <entity_name>|*|<unique_field_name>|sha256(field_value) => actual item key
-- * <entity_name>|<ws_id>|<unique_field_name>|sha256(field_value) => actual item key
--
-- * <entity_name>|*|<foreign_field_name>|<foreign_key>|<pk_string> => actual item key
-- * <entity_name>|<ws_id>|<foreign_field_name>|<foreign_key>|<pk_string> => actual item key
Copy link
Contributor

Choose a reason for hiding this comment

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

@chronolaw chronolaw removed the author/community PRs from the open-source community (not Kong Inc) label Oct 30, 2024
@team-gateway-bot team-gateway-bot added the author/community PRs from the open-source community (not Kong Inc) label Oct 30, 2024
@@ -359,7 +359,8 @@ local sync_emitter = {

emit_entity = function(self, entity_name, entity_data)
self.out_n = self.out_n + 1
self.out[self.out_n] = { type = entity_name , row = entity_data, version = self.sync_version, }
self.out[self.out_n] = { type = entity_name , row = entity_data, version = self.sync_version,
ws_id = kong.default_workspace, }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here ws_id is a fallback, if row(entity) has the field ws_id, do_sync() will use it but not delta.ws_id.

if not res then
return nil, err
end
end

local res, err = insert_entity_for_txn(t, delta_type, delta_row, nil)
local res, err = insert_entity_for_txn(t, delta_type, delta_row, opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

row.ws_id ? delta.ws_id?

Copy link
Contributor Author

@chronolaw chronolaw Oct 30, 2024

Choose a reason for hiding this comment

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

In hooks.lua, delta item will always has the field ws_id, so opts.workspace is a fallback.

  local deltas = {
    {
      type = name,
      pk = pk,
      ws_id = ws_id,
      entity = is_delete and ngx_null or entity,
    },
  }

chronolaw and others added 25 commits October 31, 2024 17:48
11-declarative_lmdb_spec.lua

fix spec/01-unit/01-db/10-declarative_spec.lua
* fix(clustering/sync): set ws_id in do_sync()

* comments
* fix(clustering/sync): delta.version may be null

* isempty

* default_ws_changed
* rename db id to pk

* change id to pk in Lua

* change pk to json

* schema:extract_pk_values
* change db filed name

* rename in rpc.sync

* rename in export
…#13815)

* fix(incremental sync): fix cache_key handling for select_by_cache_key

* fix some comments

* fix: remove unnecessary comment
* refactor(dbless): clean logic of selec_by_field

* check schema_field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/clustering core/db/migrations core/db schema-change-noteworthy size/L skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants