Skip to content

KAFKA-18662: Return CONCURRENT_TRANSACTIONS on produce request in TV2 #18733

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
Merged
merged 3 commits into from
Jan 29, 2025
Merged

KAFKA-18662: Return CONCURRENT_TRANSACTIONS on produce request in TV2 #18733

merged 3 commits into from
Jan 29, 2025

Conversation

CalvinConfluent
Copy link
Contributor

@github-actions github-actions bot added triage PRs from the community core Kafka Broker producer clients labels Jan 29, 2025
@jolshan
Copy link
Member

jolshan commented Jan 29, 2025

Thanks Calvin! Can we also add the error to the list in ProduceResponse.java?

jolshan
jolshan reviewed Jan 29, 2025
View reviewed changes
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
@@ -115,7 +115,7 @@ class ReplicaManagerTest {
private var mockRemoteLogManager: RemoteLogManager = _
private var addPartitionsToTxnManager: AddPartitionsToTxnManager = _
private var brokerTopicStats: BrokerTopicStats = _
private val transactionSupportedOperation = genericErrorSupported
private var transactionSupportedOperation: TransactionSupportedOperation = genericErrorSupported
Copy link
Member

Choose a reason for hiding this comment

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

Is this only used in each test once? If so maybe it doesn't need to be a global var. If it's just one that uses a variant, we can define it separately in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

jolshan
jolshan reviewed Jan 29, 2025
View reviewed changes
core/src/main/scala/kafka/server/ReplicaManager.scala
Errors.COORDINATOR_LOAD_IN_PROGRESS |
Errors.COORDINATOR_NOT_AVAILABLE |
Errors.NOT_COORDINATOR => Some(new NotEnoughReplicasException(
s"Unable to verify the partition has been added to the transaction. Underlying error: ${error.toString}"))
case Errors.CONCURRENT_TRANSACTIONS =>
if (transactionSupportedOperation != addPartition) {
Copy link
Member

Choose a reason for hiding this comment

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

I have a PR open that can make this check easier. Depending on what goes in first, we can modify. 👍

@jolshan
Copy link
Member

jolshan commented Jan 29, 2025

Thanks Calvin! Looks pretty good. Just left a few minor comments.

@github-actions github-actions bot removed the triage PRs from the community label Jan 29, 2025
@CalvinConfluent
Copy link
Contributor Author

testAdminClientApisAuthenticationFailure and testWakeupAfterSyncGroupReceivedExternalCompletion are flaky tests.

jolshan
jolshan approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@jolshan jolshan left a comment

Choose a reason for hiding this comment

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

Thanks Calvin!

@jolshan jolshan merged commit a3b34c1 into apache:trunk Jan 29, 2025
7 of 9 checks passed
jolshan pushed a commit that referenced this pull request Jan 29, 2025
@CalvinConfluent @jolshan
KAFKA-18662: Return CONCURRENT_TRANSACTIONS on produce request in TV2 (…
c61cf07 
…#18733)

While testing, it was found that the not_enough_replicas error was super common and could be easily confused. Since we are already bumping the request, we can signify that the produce request may return this error and new clients can handle it 

(Note, the java client should be able to handle this already as a retriable error, but other client libraries may need to implement this change)

Reviewers: Justine Olshan <[email protected]>
pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
@CalvinConfluent
KAFKA-18662: Return CONCURRENT_TRANSACTIONS on produce request in TV2 (…
ae3b26a 
…apache#18733)

While testing, it was found that the not_enough_replicas error was super common and could be easily confused. Since we are already bumping the request, we can signify that the produce request may return this error and new clients can handle it 

(Note, the java client should be able to handle this already as a retriable error, but other client libraries may need to implement this change)

Reviewers: Justine Olshan <[email protected]>
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
@CalvinConfluent @manoj-mathivanan
KAFKA-18662: Return CONCURRENT_TRANSACTIONS on produce request in TV2 (…
e8344b2 
…apache#18733)

While testing, it was found that the not_enough_replicas error was super common and could be easily confused. Since we are already bumping the request, we can signify that the produce request may return this error and new clients can handle it 

(Note, the java client should be able to handle this already as a retriable error, but other client libraries may need to implement this change)

Reviewers: Justine Olshan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jolshan jolshan jolshan approved these changes

Assignees
No one assigned
Labels
clients core Kafka Broker producer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CalvinConfluent @jolshan