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(router/atc): cleanup split routes and services #12990

Merged
merged 2 commits into from
May 14, 2024

Conversation

bungle
Copy link
Member

@bungle bungle commented May 7, 2024

Summary

Just a slightly tighter code and reusing table.

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

This was originally proposed at #12901 for inclusion in #12875.

REBASE AND MERGE

@github-actions github-actions bot added core/router cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels May 7, 2024
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from 241f83e to 4d700ce Compare May 7, 2024 10:12
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 7, 2024
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch 5 times, most recently from 7efba1d to 8fae848 Compare May 7, 2024 14:01
@pull-request-size pull-request-size bot added size/M and removed size/L labels May 7, 2024
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from 8fae848 to d60ad2e Compare May 7, 2024 14:04
@bungle bungle marked this pull request as ready for review May 7, 2024 14:04
@bungle bungle requested a review from chronolaw May 7, 2024 14:04
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from d60ad2e to 199a117 Compare May 7, 2024 14:09
kong/router/transform.lua Outdated Show resolved Hide resolved
kong/router/transform.lua Outdated Show resolved Hide resolved
kong/router/transform.lua Show resolved Hide resolved
@chronolaw
Copy link
Contributor

This PR has almost the same logic as the original code, but it also introduces two extra tables, the complex data structure make the code hard to understand, we should add more comments for future's maintenance.

@bungle
Copy link
Member Author

bungle commented May 9, 2024

This PR has almost the same logic as the original code, but it also introduces two extra tables, the complex data structure make the code hard to understand, we should add more comments for future's maintenance.

It has less code than before and is also more performant, does less new table creations and clones, does not use pairs for looping. (On a potentially large 100.000 iteration loop).

The exact same statement can be said on your original refactor.

@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from 199a117 to 0ec9b25 Compare May 14, 2024 07:07
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 14, 2024
@bungle
Copy link
Member Author

bungle commented May 14, 2024

This PR has almost the same logic as the original code, but it also introduces two extra tables, the complex data structure make the code hard to understand, we should add more comments for future's maintenance.

@chronolaw comments were added. I also added another commit to clear the transformation functions.

@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from 0ec9b25 to 8d50e0e Compare May 14, 2024 07:11
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch 2 times, most recently from 924b77e to 069d44f Compare May 14, 2024 08:11
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from 069d44f to da2ef2e Compare May 14, 2024 08:13
Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
…x typos

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@bungle bungle force-pushed the refactor/cleanup-split-routes-and-services branch from da2ef2e to db4e68d Compare May 14, 2024 08:15
@chronolaw chronolaw changed the title refactor(router): cleanup split routes and services refactor(router/atc): cleanup split routes and services May 14, 2024
Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

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

Great job, thank you Aapo.

kong/router/transform.lua Outdated Show resolved Hide resolved
@bungle bungle merged commit 5da4b6b into master May 14, 2024
27 checks passed
@bungle bungle deleted the refactor/cleanup-split-routes-and-services branch May 14, 2024 11:00
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/router size/L skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants