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(runloop/balancer): clean the code of round robin algo #11315

Merged
merged 8 commits into from
Sep 13, 2023
Merged

refactor(runloop/balancer): clean the code of round robin algo #11315

merged 8 commits into from
Sep 13, 2023

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Jul 28, 2023 •
edited
Loading

Summary

Did some style clean for the code of round-robin algo.

Checklist

Full changelog

  • [Implement ...]

Issue reference

Fix #[issue number]

@chronolaw chronolaw marked this pull request as draft July 28, 2023 08:46
@chronolaw chronolaw marked this pull request as ready for review July 28, 2023 10:16
@dndx dndx force-pushed the style/clean_round_robin_algo branch from 99936a8 to 6780856 Compare August 10, 2023 06:19
@dndx dndx requested a review from locao August 10, 2023 06:19
@dndx
Copy link
Member

dndx commented Aug 10, 2023

@locao could you take a look?

AndyZhang0707
AndyZhang0707 reviewed Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@AndyZhang0707 AndyZhang0707 left a comment

Choose a reason for hiding this comment

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

LGTM.

@vm-001 vm-001 force-pushed the style/clean_round_robin_algo branch from 74bcaa8 to cdf2130 Compare August 29, 2023 10:21
@locao
Copy link
Contributor

locao commented Aug 29, 2023

I will not oppose merging this change, but I am not a supporter of fixing what is not broken. I don't think the new code is more readable than before. Is there a performance-wise improvement?

@chronolaw
Copy link
Contributor Author

I will not oppose merging this change, but I am not a supporter of fixing what is not broken. I don't think the new code is more readable than before. Is there a performance-wise improvement?

Sure, in this line (https://github.com/Kong/kong/pull/11315/files#diff-79ad4c6a67afdb7cfead0d69cfd9ce58d017d600b5a5456d5cc144923699e827R66) we append array faster.

chobits
chobits approved these changes Aug 31, 2023
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.

This pr preserved the original functionality

@chronolaw
Copy link
Contributor Author

@locao, Could you take a look and approve it? thanks.

@kikito kikito requested a review from oowl September 12, 2023 08:45
@windmgc
Copy link
Member

windmgc commented Sep 13, 2023

It feels like a style type is more suitable for this PR instead of a refactor

@chronolaw
Copy link
Contributor Author

It feels like a style type is more suitable for this PR instead of a refactor

It's ok to me.

@windmgc windmgc merged commit 4bdae54 into master Sep 13, 2023
29 of 30 checks passed
@windmgc windmgc deleted the style/clean_round_robin_algo branch September 13, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndyZhang0707 AndyZhang0707 AndyZhang0707 left review comments

@oowl oowl oowl approved these changes

@chobits chobits chobits approved these changes

@locao locao Awaiting requested review from locao

Assignees

@chronolaw chronolaw

Labels
core/balancer size/M skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chronolaw @dndx @locao @windmgc @chobits @AndyZhang0707 @oowl