Skip to content

KAFKA-12620 Allocate Producer IDs in KRaft controller#10752

Merged
cmccabe merged 5 commits intoapache:trunkfrom
mumrah:kafka-12620-producer-ids-kraft-2
Jun 3, 2021
Merged

KAFKA-12620 Allocate Producer IDs in KRaft controller#10752
cmccabe merged 5 commits intoapache:trunkfrom
mumrah:kafka-12620-producer-ids-kraft-2

Conversation

@mumrah
Copy link
Copy Markdown
Member

@mumrah mumrah commented May 24, 2021

This is part 2 of KIP-730, part 1 was in #10504.

This PR adds support on the KRaft controller for handling AllocateProducerIDs requests and managing the state of the latest producer ID block in the controller and committing this state to the metadata log.

Comment thread core/src/main/scala/kafka/server/BrokerServer.scala Outdated
Comment thread core/src/main/scala/kafka/server/BrokerServer.scala
Comment thread core/src/main/scala/kafka/server/metadata/BrokerMetadataListener.scala Outdated
@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented May 26, 2021

Thanks for this, @mumrah . Very clean PR overall.

Can you add a test that the RPC fails if we don't have cluster authorization, in ControllerApisTest.scala? Should be very similar to testUnauthorizedHandleAlterPartitionReassignments. I think it's a good idea to have something like this for all the controller RPCs to prevent authorization regressions.

Also, QuorumControllerTest#testSnapshotSaveAndLoad needs to be updated... it's failing now in the PR.

AllocateProducerIdsRequestData request) {
return appendWriteEvent("allocateProducerIds",
() -> producerIdControlManager.generateNextProducerId(request.brokerId(), request.brokerEpoch()))
.thenApply(resultOrError -> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can see the reasoning behind doing it this way, but for all the other RPCs we've just been letting the future complete as an error, and making the caller handle it. One issue with changing the pattern, I suppose, is that not all the controller functions return an RPC data structure that allows setting an error. So let's keep the current pattern here for now, where caller has to handle the future completing exceptionally.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds fine. I'll change the return type of generateNextProducerId to just be ControllerResult<ProducerIdBlock> and throw ApiExceptions directly instead.

* Adding integration tests for authz
* Fixing failing unit test (and adding case for ProducerIdsRecord)
Copy link
Copy Markdown
Contributor

@cmccabe cmccabe 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, @mumrah

@cmccabe cmccabe merged commit e97cff2 into apache:trunk Jun 3, 2021
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.

2 participants