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

Strategy ordering bug fix #3818

Merged
merged 5 commits into from Apr 26, 2022
Merged

Strategy ordering bug fix #3818

merged 5 commits into from Apr 26, 2022

Conversation

jliunyu
Copy link
Contributor

@jliunyu jliunyu commented Apr 19, 2022

If multiple strategies are supported, the first strategy would be used. But range one is always used at this moment. This PR will fixed this issue.

TESTS: TESTS=0132 ./run-test.sh valgrind

[0132_strategy_ordering      / 12.606s] ================= Test 0132_strategy_ordering PASSED =================
[<MAIN>                      / 13.410s] ALL-TESTS: duration 13393.378ms
TEST 20220419071425 (valgrind, scenario default) SUMMARY
#==================================================================#
| <MAIN>                                   |     PASSED |  13.411s |
| 0132_strategy_ordering                   |     PASSED |  12.604s |
#==================================================================#
[<MAIN>                      / 14.009s] 0 thread(s) in use by librdkafka
[<MAIN>                      / 14.010s] 
============== ALL TESTS PASSED ==============
==1963337== 
==1963337== FILE DESCRIPTORS: 4 open at exit.
==1963337== Open file descriptor 63:
==1963337==    <inherited from parent>
==1963337== 
==1963337== Open file descriptor 2: /dev/pts/0
==1963337==    <inherited from parent>
==1963337== 
==1963337== Open file descriptor 1: /dev/pts/0
==1963337==    <inherited from parent>
==1963337== 
==1963337== Open file descriptor 0: /dev/pts/0
==1963337==    <inherited from parent>
==1963337== 
==1963337== 
==1963337== HEAP SUMMARY:
==1963337==     in use at exit: 0 bytes in 0 blocks
==1963337==   total heap usage: 3,529 allocs, 3,529 frees, 1,369,295 bytes allocated
==1963337== 
==1963337== All heap blocks were freed -- no leaks are possible
==1963337== 
==1963337== For lists of detected and suppressed errors, rerun with: -s
==1963337== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
###
###  ./test-runner in valgrind mode PASSED! ###

edenhill
edenhill suggested changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Good stuff!
Don't forget to add a CHANGELOG Fixed entry

src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
tests/0132-strategy_ordering.c Outdated Show resolved Hide resolved
tests/0132-strategy_ordering.c Outdated Show resolved Hide resolved
tests/0132-strategy_ordering.c Outdated Show resolved Hide resolved
tests/0132-strategy_ordering.c Outdated Show resolved Hide resolved
tests/0132-strategy_ordering.c
}


int main_0132_strategy_ordering(int argc, char **argv) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good tests! 👍

@jliunyu jliunyu marked this pull request as ready for review April 20, 2022 04:30
edenhill
edenhill suggested changes Apr 20, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c Outdated Show resolved Hide resolved
src/rdkafka_assignor.c
* and the strategies earlier in the list have higher priority
* when retrieve the protocol. */
rd_list_sort(&rk->rk_conf.partition_assignors,
rd_kafka_assignor_cmp_idx);

/* Clear the SORTED flag because the list is sorted according to the
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

jliunyu and others added 3 commits April 20, 2022 11:50
@jliunyu @edenhill
Update CHANGELOG.md
80cf96b 
Co-authored-by: Magnus Edenhill <magnus@edenhill.se>
@jliunyu @edenhill
Update src/rdkafka_assignor.c
cfa23c8 
Co-authored-by: Magnus Edenhill <magnus@edenhill.se>
@jliunyu @edenhill
Update src/rdkafka_assignor.c
08ca46a 
Co-authored-by: Magnus Edenhill <magnus@edenhill.se>
edenhill
edenhill approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks Jing!

edenhill
edenhill reviewed Apr 26, 2022
View reviewed changes
tests/test.c
@@ -476,6 +477,7 @@ struct test tests[] = {
_TEST(0129_fetch_aborted_msgs, 0, TEST_BRKVER(0, 11, 0, 0)),
_TEST(0130_store_offsets, 0),
_TEST(0131_connect_timeout, TEST_F_LOCAL),
_TEST(0132_strategy_ordering, 0, TEST_BRKVER(2, 4, 0, 0)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is broker version 2.4 needed? I believe consumer groups were added in 0.9.0

@edenhill edenhill merged commit f053117 into confluentinc:master Apr 26, 2022
@jliunyu jliunyu deleted the strategyorder branch April 26, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edenhill edenhill edenhill approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jliunyu @edenhill