Elect leader api(KIP-460) implemented #1311

PratRanj07 · 2024-10-03T18:10:34Z

Implemented the ElectLeaders API and wrote unit tests for it

confluent-cla-assistant · 2024-10-03T18:10:43Z

🎉 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.}

milindl

Looks pretty good

Please add an integration test

milindl · 2024-10-08T08:10:01Z

examples/.gitignore

@@ -43,3 +43,4 @@ protobuf_consumer_example/protobuf_consumer_example
 protobuf_producer_example/protobuf_producer_example
 stats_example/stats_example
 transactions_example/transactions_example
+admin_elect_leaders/admin_elect_leaders


Add with all the other admin stuff (in alphabetical order)

milindl · 2024-10-08T08:10:25Z

examples/README.md

@@ -86,6 +86,8 @@ Examples

  [transactions_example](transactions_example) - Showcasing a transactional consume-process-produce application

+  [admin_elect_leaders](admin_elect_leaders) - Perform Preferred/ Unclean elections for the specified Topic Partitions


Suggested change

[admin_elect_leaders](admin_elect_leaders) - Perform Preferred/ Unclean elections for the specified Topic Partitions

[admin_elect_leaders](admin_elect_leaders) - Perform Preferred/Unclean elections for the specified Topic Partitions

And also keep it in alphabetical order

milindl · 2024-10-08T08:23:48Z

kafka/adminapi.go

+		return result, newErrorFromString(ErrInvalidArg, "expected non-empty partitions")
+	}
+
+	result = ElectLeadersResult{}


nit: not needed

milindl · 2024-10-08T08:42:56Z

kafka/adminapi.go

+	partitions = make([]TopicPartition, partCnt)
+
+	for i := 0; i < partCnt; i++ {
+		setupTopicPartitionFromCtopicPartitionResult(&partitions[i],


Refactor this method so that you can just pass the C.rd_kafka_topic_partition_result_t directly into it and let it call the other on its own.

I also feel like both newTopicPartitionsFromCTopicPartitionResult and setupTopicPartitionFromCtopicPartitionResult should be in kafka.go, but that can wait for when we need it from another place.

milindl · 2024-10-08T08:45:16Z

kafka/adminapi.go

+// err returns any top level level that occured during the operation.
+func (a *AdminClient) ElectLeaders(ctx context.Context, electLeaderRequest ElectLeadersRequest, options ...ElectLeadersAdminOption) (result ElectLeadersResult, err error) {
+
+	if len(electLeaderRequest.partitions) == 0 {


As we discussed in the call with the team:

0-length should give an error

nil corresponds to null and should set for all partitions

But len(nil) == len([]) == 0

milindl · 2024-10-08T09:07:42Z

kafka/adminapi.go

+	partitions []TopicPartition
+}
+
+func NewElectLeadersRequest(electionType ElectionType, partitions []TopicPartition) ElectLeadersRequest {


Add a comment above function

milindl · 2024-10-08T09:09:19Z

kafka/adminapi.go

+	// TopicPartitions for which election has been performed and specific error if any
+	// that occured while election in the specific TopicPartition.


Suggested change

// TopicPartitions for which election has been performed and specific error if any

// that occured while election in the specific TopicPartition.

// TopicPartitions for which election has been performed and the per-partition error, if any

// that occurred while running the election for the specific TopicPartition.

milindl · 2024-10-08T09:14:25Z

kafka/adminapi.go

+// User has to check all the elements of the result to check any error occured per partition.
+// err returns any top level level that occured during the operation.


Suggested change

// User has to check all the elements of the result to check any error occured per partition.

// err returns any top level level that occured during the operation.

// Individual TopicPartitions inside the ElectLeadersResult should be checked for errors.

// Additionally, an error that is not nil for client-level errors is returned.

milindl · 2024-10-08T09:17:51Z

kafka/adminapi_test.go

+		{},
+		{SetAdminRequestTimeout(time.Second)},
+	} {
+		// empty list should fail


Change this after the librdkafka change, so the empty list fails, but a nil list times out instead

Actually after the librdkafka change, both empty list and nil partitions is valid.
If there is an empty list in input, we are getting an empty list in the output and for nil, all partitions.

milindl · 2024-10-08T09:22:20Z

examples/admin_elect_leaders/admin_elect_leaders.go

+
+	res, err := ac.ElectLeaders(ctx, kafka.NewElectLeadersRequest(electionType, topicPartitionList))
+	if err != nil {
+		kafkaErr, ok := err.(kafka.Error) // Type assertion


Suggested change

kafkaErr, ok := err.(kafka.Error) // Type assertion

kafkaErr, ok := err.(kafka.Error)

emasab

Great PR Pratyush! A few comments:

examples/README.md

examples/admin_elect_leaders/admin_elect_leaders.go

kafka/adminapi.go

emasab

LGTM. Well done Pratyush!

Milind is OOO

PratRanj07 requested review from a team as code owners October 3, 2024 18:10

airlock-confluentinc bot force-pushed the dev_electLeaders branch from 6705733 to c7960d9 Compare October 8, 2024 07:19

milindl previously requested changes Oct 8, 2024

View reviewed changes

PratRanj07 added 6 commits October 8, 2024 22:53

elect leaders feature

22eb307

new changes from librdkafka

5f01c3d

indentation changes

d6ce98e

required changes acc to librdkafka

02c424b

formatting

c82ca90

requested changes

7c3f979

airlock-confluentinc bot force-pushed the dev_electLeaders branch from c7960d9 to 7c3f979 Compare October 8, 2024 17:23

PratRanj07 requested a review from milindl October 8, 2024 19:52

PratRanj07 added 3 commits October 9, 2024 11:21

changelog entry

b34204d

changelog entry changed

40efd4e

changelog entry change

c35f5d5

emasab requested changes Oct 9, 2024

View reviewed changes

Requested changes

1313aaa

PratRanj07 requested a review from emasab October 9, 2024 20:00

PratRanj07 added 2 commits October 10, 2024 19:25

Merge master

d172dd4

documentation change

36a8ffc

emasab approved these changes Oct 10, 2024

View reviewed changes

PratRanj07 merged commit be6f062 into master Oct 10, 2024
3 checks passed

PratRanj07 deleted the dev_electLeaders branch October 10, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elect leader api(KIP-460) implemented #1311

Elect leader api(KIP-460) implemented #1311

PratRanj07 commented Oct 3, 2024

confluent-cla-assistant bot commented Oct 3, 2024

milindl left a comment

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

milindl Oct 8, 2024

PratRanj07 Oct 8, 2024

milindl Oct 8, 2024

emasab left a comment

emasab left a comment

		@@ -86,6 +86,8 @@ Examples

		[transactions_example](transactions_example) - Showcasing a transactional consume-process-produce application

		[admin_elect_leaders](admin_elect_leaders) - Perform Preferred/ Unclean elections for the specified Topic Partitions

		// TopicPartitions for which election has been performed and specific error if any
		// that occured while election in the specific TopicPartition.

		// User has to check all the elements of the result to check any error occured per partition.
		// err returns any top level level that occured during the operation.

	kafkaErr, ok := err.(kafka.Error) // Type assertion
	kafkaErr, ok := err.(kafka.Error)

Elect leader api(KIP-460) implemented #1311

Elect leader api(KIP-460) implemented #1311

Conversation

PratRanj07 commented Oct 3, 2024

confluent-cla-assistant bot commented Oct 3, 2024

milindl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emasab left a comment

Choose a reason for hiding this comment

emasab left a comment

Choose a reason for hiding this comment