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

Added Public API for Preferred/Unclean Leader Election on Topic Partitions #4845

Merged
merged 43 commits into from
Oct 9, 2024

Conversation

PratRanj07
Copy link
Contributor

This PR introduces the public API rd_kafka_ElectLeaders, enabling the execution of Preferred or Unclean leader election for specific topic partitions. Additionally, unit tests are also included.

@PratRanj07 PratRanj07 requested a review from a team as a code owner September 11, 2024 06:13
@pranavrth
Copy link
Member

Please use plural form of elect leader i.e. elect leaders instead of elect leader. Its all over the changes. Fix those. For example RD_KAFKA_EVENT_ELECTLEADER_RESULT instead of RD_KAFKA_EVENT_ELECTLEADERS_RESULT

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

First pass of implementation excluding tests and examples.

src/rdkafka.h Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka_aux.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.h Outdated Show resolved Hide resolved
src/rdkafka_aux.c Show resolved Hide resolved
rd_kafka_t *rk,
rd_kafka_queue_t *useq,
int with_options,
int etype) {
Copy link
Member

Choose a reason for hiding this comment

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

Take rd_kafka_ElectionType_t input directly.

@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Another round of review. Example and Test remaining.

src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka_aux.c Outdated Show resolved Hide resolved
src/rdkafka_aux.c Outdated Show resolved Hide resolved
src/rdkafka_aux.h Outdated Show resolved Hide resolved
src/rdkafka_op.h Outdated Show resolved Hide resolved
src/rdkafka_request.c Outdated Show resolved Hide resolved
@pranavrth
Copy link
Member

Please add following documentations.

  1. Add CHANGELOG
  2. Add this KIP to the supported list of KIPs in INTRODUCTION.md

@pranavrth
Copy link
Member

Please also ensure there are no memory leaks.

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Another round of review.

src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
Comment on lines 4179 to 4182
result->err = err;
rd_list_init_copy(&result->partitions, partitions);
rd_list_copy_to(&result->partitions, partitions, NULL, NULL);
return result;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not using rd_list_copy directly here?

src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_aux.c Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Almost there with the code and documentation part.

examples/elect_leaders.c Show resolved Hide resolved
src/rdkafka.h Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Show resolved Hide resolved
src/rdkafka_aux.h Outdated Show resolved Hide resolved
src/rdkafka_request.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka.h Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka_aux.c Outdated Show resolved Hide resolved
src/rdkafka_aux.h Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_aux.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Comments for example.

examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

For the test

tests/0080-admin_ut.c Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Few minor changes.

src/rdkafka_admin.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

LGTM!
Great Work Pratyush!.

src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
Comment on lines 9368 to 9375
/* Non empty topic_partition_list should be present */
if (elect_leaders->partitions->cnt == 0) {
rd_kafka_admin_result_fail(rko, RD_KAFKA_RESP_ERR__INVALID_ARG,
"No partitions specified");
rd_kafka_admin_common_worker_destroy(rk, rko,
rd_true /*destroy*/);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I was going through the Java implementation and figured out that null list is also allowed which will trigger leader election for all the topic partitions. We need to change the code to support this. There maybe multiple places we need to do this change.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/Admin.java#L1040-L1041

Topic Partition is nullable - https://github.com/apache/kafka/blob/trunk/clients/src/main/resources/common/message/ElectLeadersRequest.json#L29

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Thanks Pratyush, left these comments

INTRODUCTION.md Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_admin.c Outdated Show resolved Hide resolved
src/rdkafka_aux.c Outdated Show resolved Hide resolved
src/rdkafka_request.c Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
tests/0080-admin_ut.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
examples/elect_leaders.c Show resolved Hide resolved
INTRODUCTION.md Outdated Show resolved Hide resolved
src/rdkafka.h Outdated Show resolved Hide resolved
examples/elect_leaders.c Outdated Show resolved Hide resolved
@PratRanj07 PratRanj07 requested review from emasab and pranavrth October 9, 2024 08:57
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Pratyush!

Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

Great Work Pratyush!
LGTM!.

@PratRanj07 PratRanj07 merged commit 0cd23c6 into master Oct 9, 2024
3 checks passed
@PratRanj07 PratRanj07 deleted the dev_electLeaders branch October 9, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants