Add support for alter/list partition reassignements APIs #1617

sladkoff · 2020-02-17T15:53:50Z

We added support for API Keys 45 (AlterPartitionReassignments) and 46 (ListPartitionReassignments).

This enables sarama to update the replication factor of topics for Kafka 2.4.0.0+ (See also #1238).

We had to do change quite a lot since these APIs require the use of newer data structures in the body as well as in the header of the requests/responses (mainly compact structures and tagged fields).

Please let us know what you think.

dnwe · 2020-02-19T22:42:47Z

@sladkoff 🌟 thanks for this, great PR!

It'll take a little while to review it all as I also need to brush up my knowledge on KIP-482 and how this fits with the existing messages too.

bai · 2020-02-20T03:45:21Z

@sladkoff 💯 Do you think you could rebase your branch to make sure it's ✅on CI 🙏

dnwe

Thanks again, the protocol changes look good 👍

A small number of enquiries below:

dnwe · 2020-02-21T22:20:43Z

admin.go

+	}
+
+	request := &AlterPartitionReassignmentsRequest{
+		TimeoutMs: int32(10000),


Where did this 10_000 millisecond default timeout come from? The protocol description suggests 60_000 should be the default?

https://github.com/apache/kafka/blob/2.4.0/clients/src/main/resources/common/message/AlterPartitionReassignmentsRequest.json#L23

we probably just put something in there :) Using the suggested default makes sense. I added this.

dnwe · 2020-02-21T22:21:39Z

admin.go

+	}
+
+	request := &ListPartitionReassignmentsRequest{
+		TimeoutMs: int32(10000),


As above, shouldn't this be 60_000 ?

dnwe · 2020-02-21T22:28:27Z

admin_test.go

+	seedBroker.SetHandlerByMap(map[string]MockResponse{
+		"MetadataRequest": NewMockMetadataResponse(t).
+			SetController(seedBroker.BrokerID()).
+			SetBroker(seedBroker.Addr(), seedBroker.BrokerID()),


Please could you add a second mockBroker to the test and set that as the controller in the mock metadata response and not the seed broker? That should exercise that the request must be sent to the Controller and not just any broker (or the broker currently connected to)

dnwe · 2020-02-21T22:28:58Z

admin_test.go

+	seedBroker.SetHandlerByMap(map[string]MockResponse{
+		"MetadataRequest": NewMockMetadataResponse(t).
+			SetController(seedBroker.BrokerID()).
+			SetBroker(seedBroker.Addr(), seedBroker.BrokerID()),


As above, please use different MockBroker for the Controller

dnwe · 2020-02-21T22:33:59Z

admin.go

+
+	request.AddBlock(topic, partitions)
+
+	b, err := ca.findAnyBroker()


I thought that ListPartitionReassignments also needed to be sent to the Controller (not just any broker) like the Alter request? The Java Admin client seems to do so here

good catch. I fixed that

ghost · 2020-02-25T14:21:44Z

@dnwe I added commits which address your remarks. Can you have a look? One of the CI jobs failed, but it does not look related to our change, right?

dnwe

Updated changes look good to me.

One small remaining fix below and then I think the PR is ready to be approved and merged

dnwe · 2020-02-25T15:10:04Z

admin.go

@@ -512,7 +512,7 @@ func (ca *clusterAdmin) ListPartitionReassignments(topic string, partitions []in

 	request.AddBlock(topic, partitions)

-	b, err := ca.findAnyBroker()
+	b, err := ca.Controller()


Now this is using the controller you probably want to wrapper the body in a return ca.retryOnError(isErrNoController, ...) call like you did for AlterPartitionReassignments so it refreshes the cached controller if it is stale

the retryOnError function cannot be used when a value needs to be returned. other functions in admin.go where a controller is used and a return type is needed also don't use retrying. How should this be handled? should we implement a retry function that returns something like interface{}?

@dwi-di ah of course — let's just leave this for now and we can always follow-up in another PR

dnwe · 2020-02-27T13:44:57Z

One of the CI jobs failed, but it does not look related to our change, right?

I'm guessing it was just a slight flake — but it was in the new TestListPartitionReassignmentRequest func:

--- FAIL: TestListPartitionReassignmentRequest (0.00s)
##[error]    request_test.go:63: Encoding two blocks failed
        got  [0 0 39 16 3 7 116 111 112 105 99 50 3 0 0 0 1 0 0 0 2 0 6 116 111 112 105 99 2 0 0 0 0 0 0] 
        want [0 0 39 16 3 6 116 111 112 105 99 2 0 0 0 0 0 7 116 111 112 105 99 50 3 0 0 0 1 0 0 0 2 0 0]

dnwe · 2020-02-27T13:47:36Z

I was able to re-create with:

go test -run '^TestListPartitionReassignmentRequest$' -count=500 -race

dnwe · 2020-02-27T13:54:13Z

Ah I can see the problem, the ordering of the two blocks in your check of testRequest(t, "two blocks", request, listPartitionReassignmentsRequestTwoBlocks) isn't guaranteed, so sometimes you get "topic" followed by "topic2" in your response, and sometimes you get "topic2" followed by "topic" in your response.

ghost · 2020-02-27T15:18:46Z

ah I see, but what do you propose to do here? the only way to fix it is to change the encoder to sort the blocks based on topic name, right? but that does not feel right, because it is not really needed, only for the test...

dnwe · 2020-03-01T21:48:29Z

ah I see, but what do you propose to do here? the only way to fix it is to change the encoder to sort the blocks based on topic name, right? but that does not feel right, because it is not really needed, only for the test...

Yes I think it just isn't possible to use the existing testRequest helper which is what is doing the byte-comparison along the way. In your two-block test you'd probably just test a struct --> protocolEncode --> protocolDecode --> struct roundtrip correctly gives you a pair of structs that are DeepEqual — which is essentially a slimmed down version of the existing helpers

ghost · 2020-03-04T14:01:36Z

I modified the flaky test, that should be all then.

dnwe

Thanks, LGTM 👍

…ignment * feature/update-replication-assignment: feat: Add alter and list partition reassignments

dnwe · 2020-03-06T11:31:25Z

Hmm I squash merged this manually from the CLI, but GitHub doesn't seem to have tracked the merge (2d2326e via 9501120)

sladkoff · 2020-03-10T09:52:58Z

@dnwe Nice! Can we expect to get these changes into a release soon?

d-rk · 2020-03-12T19:30:49Z

we should probably wait for #1640 before releasing this, otherwise the list partition replica assignement status is not really usable.

sladkoff · 2020-05-06T07:49:01Z

Hi guys! It's been two months now, so I'll allow myself to re-post my question 😅

Could we get an ETA for a new release with this these features?

@dnwe @bai

bai · 2020-05-06T07:50:19Z

That's fair. @dnwe any objections on cutting release today?

dnwe · 2020-05-06T07:53:54Z

@bai yes I was merging a few PRs in yesterday as I think we do need to cut a release with this and also to include @KJTsanaktsidis's fix for the session ID cache exhaustion that was introduced in 1.26 and is a nasty bug.

dnwe · 2020-05-06T07:54:33Z

@bai we might need to check on the github actions builds though, as they've been a bit flakey recently. Not sure if we can do anything to improve them

bai · 2020-05-06T07:59:09Z

I think we can address GitHub Actions issue short-term by reducing build matrix — it's gotten so much flakier with 9 builds instead of 6.

Hopefully when we have that docker-compose setup in place it'll get better 🙏

Want to do this release yourself or would you prefer me cutting it?

dnwe · 2020-05-06T09:53:12Z

Sure I can give a go to cutting the release. Just a tag in Git and then create a release with a small changelog linking to the PRs?

bai · 2020-05-06T11:07:02Z

@dnwe I usually use Draft New Release button on Releases page here: https://github.com/Shopify/sarama/releases.
Once I'm confident (title/tag look what I want them to look) I'm promoting Draft Release with "Publish Release" button.

So basically I'm not doing manual git tag but use GitHub UI to release 🤷‍♀️

dnwe · 2020-05-06T14:09:33Z

@sladkoff released and announced!

https://github.com/Shopify/sarama/releases/tag/v1.26.2

https://twitter.com/oldmanuk/status/1258035278771621888

(apologies for missing shoutouts to those I couldn't immediately find on Twitter)

sladkoff · 2020-05-06T14:36:09Z

@dnwe @bai Awesome 🎉 Thanks for the quick reaction!

bai · 2020-05-06T14:42:48Z

Props to Dominic for cutting this one and all of you folks for contributing 🙏

sladkoff requested a review from bai as a code owner February 17, 2020 15:53

ghost added the cla-needed label Feb 17, 2020

sladkoff changed the title ~~Feature/update replication assignment~~ Add support for alter/list partition reassignements APIs Feb 17, 2020

d1egoaz requested a review from dnwe February 19, 2020 13:31

ghost removed the cla-needed label Feb 20, 2020

sladkoff and others added 9 commits February 20, 2020 08:07

Add alter partition reassignments API

6d09837

add headerVersion for all requests

f9334fc

Implement AlterPartitionReassignmentsResponse decoder

d926c43

add tests for alter_partition_reassignments

24f957e

add test for alter_partition_reassignments_response

675d283

Pretty print partition reassignment errors

e2b1df8

Fix wrong offset increment

15d7fb3

Add admin tests for AlterPartitionReassignments

cd50703

add ListPartitionReassignmentsRequest

e4f9b4c

sladkoff force-pushed the feature/update-replication-assignment branch from 601e78f to 093729b Compare February 20, 2020 07:08

sladkoff and others added 7 commits February 20, 2020 08:19

Decode empty tagged fields in response header v1

bf390fb

add ListPartitionReassignmentsResponse

314db3f

add helper for putEmptyTaggedFieldArray

1cf58af

fix problem when no responseBody is provided

9ef5c3b

make sure mockbroker can handle different reponse header versions

023606a

make sure partition reassignment can be aborted

aaf9f16

add ListPartitionReassignments to admin client

c4390e1

sladkoff force-pushed the feature/update-replication-assignment branch from 093729b to c4390e1 Compare February 20, 2020 07:20

Fix linter errors

3042188

sladkoff mentioned this pull request Feb 20, 2020

Add replication-factor flag to alter topic deviceinsight/kafkactl#37

Merged

3 tasks

dnwe reviewed Feb 21, 2020

View reviewed changes

make sure only controller answers requests

ee24607

use correct default for timeout

6f230b4

dnwe reviewed Feb 25, 2020

View reviewed changes

modify test to skip byteComparison

9e0199b

dnwe approved these changes Mar 4, 2020

View reviewed changes

dnwe added a commit that referenced this pull request Mar 6, 2020

Merge pull request #1617 from sladkoff:feature/update-replication-ass…

9501120

…ignment * feature/update-replication-assignment: feat: Add alter and list partition reassignments

dnwe closed this Mar 6, 2020

sladkoff deleted the feature/update-replication-assignment branch March 10, 2020 09:52

d-rk mentioned this pull request Mar 12, 2020

Feature/fix list partition reassignments #1640

Merged

hindessm mentioned this pull request Jul 23, 2023

Add some retry logic to more admin client functions #2520

Merged

VinGarcia mentioned this pull request Nov 28, 2023

We probably need an example of how to use the AlterPartitionReassignments function #2735

Open


		request.AddBlock(topic, partitions)

		b, err := ca.findAnyBroker()

Add support for alter/list partition reassignements APIs #1617

Add support for alter/list partition reassignements APIs #1617

Conversation

sladkoff commented Feb 17, 2020 • edited Loading

dnwe commented Feb 19, 2020

bai commented Feb 20, 2020

dnwe 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

ghost commented Feb 25, 2020

dnwe 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

dnwe commented Feb 27, 2020

dnwe commented Feb 27, 2020

dnwe commented Feb 27, 2020

ghost commented Feb 27, 2020

dnwe commented Mar 1, 2020

ghost commented Mar 4, 2020

dnwe left a comment

Choose a reason for hiding this comment

dnwe commented Mar 6, 2020

sladkoff commented Mar 10, 2020

d-rk commented Mar 12, 2020

sladkoff commented May 6, 2020

bai commented May 6, 2020

dnwe commented May 6, 2020

dnwe commented May 6, 2020

bai commented May 6, 2020

dnwe commented May 6, 2020

bai commented May 6, 2020

dnwe commented May 6, 2020

sladkoff commented May 6, 2020

bai commented May 6, 2020

sladkoff commented Feb 17, 2020 •

edited

Loading