You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "jolshan (via GitHub)" <gi...@apache.org> on 2025/01/28 00:05:41 UTC
[PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
jolshan opened a new pull request, #18720:
URL: https://github.com/apache/kafka/pull/18720
Attempting the solution where we push the decision to the transaction coordinator.
I tried running some of the producer bench here, but I am still seeing some 100ms produce requests. Will update again soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936496849
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -569,20 +569,21 @@ class UnifiedLog(@volatile var logStartOffset: Long,
* Maybe create and return the VerificationGuard for the given producer ID if the transaction is not yet ongoing.
* Creation starts the verification process. Otherwise return the sentinel VerificationGuard.
*/
- def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short): VerificationGuard = lock synchronized {
+ def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short, isTV2: Boolean): VerificationGuard = lock synchronized {
Review Comment:
I left a few usages in tests since some of those are actually setting the version to TV2. But all non-test usages (except for one in the client) are changed. Let me know if you think that should be changed too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jeffkbkim (via GitHub)" <gi...@apache.org>.
jeffkbkim commented on PR #18720:
URL: https://github.com/apache/kafka/pull/18720#issuecomment-2632038120
Can we add a bit more information as to what is the nature of the perf regression? And the PR description indicates that we are planning to add a producer bench result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936348768
##########
core/src/main/scala/kafka/server/AddPartitionsToTxnManager.scala:
##########
@@ -67,10 +67,14 @@ object AddPartitionsToTxnManager {
* genericErrorSupported: This maps to the case when the clients are updated to handle the TransactionAbortableException
* addPartition: This allows the partition to be added to the transactions inflight with the Produce and TxnOffsetCommit requests. Plus the behaviors in genericErrorSupported.
*/
-sealed trait TransactionSupportedOperation
+sealed trait TransactionSupportedOperation {
+ val isTV2 = false;
Review Comment:
Hmm I think it is valid to rename. Let me think on supportsEpochBump. It might be the best option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936346455
##########
core/src/main/scala/kafka/server/AddPartitionsToTxnManager.scala:
##########
@@ -67,10 +67,14 @@ object AddPartitionsToTxnManager {
* genericErrorSupported: This maps to the case when the clients are updated to handle the TransactionAbortableException
* addPartition: This allows the partition to be added to the transactions inflight with the Produce and TxnOffsetCommit requests. Plus the behaviors in genericErrorSupported.
*/
-sealed trait TransactionSupportedOperation
+sealed trait TransactionSupportedOperation {
+ val isTV2 = false;
Review Comment:
Sorry was having a java moment 😂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #18720:
URL: https://github.com/apache/kafka/pull/18720#issuecomment-2632380819
> Can we add a bit more information as to what is the nature of the perf regression? And the PR description indicates that we are planning to add a producer bench result?
I will add some more information.
I was just using the transactional bench, but I this is part 1/2 for the improvement. I can make this clearer in the PR title as well. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654 [1/2]: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan merged PR #18720:
URL: https://github.com/apache/kafka/pull/18720
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936346953
##########
core/src/main/scala/kafka/server/ReplicaManager.scala:
##########
@@ -950,7 +958,7 @@ class ReplicaManager(val config: KafkaConfig,
): Unit = {
// Skip verification if the request is not transactional or transaction verification is disabled.
if (transactionalId == null ||
- !config.transactionLogConfig.transactionPartitionVerificationEnable
+ (!config.transactionLogConfig.transactionPartitionVerificationEnable && transactionSupportedOperation != addPartition)
Review Comment:
Yup -- there was a place in Calvin's PR I also need to update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936485613
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -569,20 +569,21 @@ class UnifiedLog(@volatile var logStartOffset: Long,
* Maybe create and return the VerificationGuard for the given producer ID if the transaction is not yet ongoing.
* Creation starts the verification process. Otherwise return the sentinel VerificationGuard.
*/
- def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short): VerificationGuard = lock synchronized {
+ def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short, isTV2: Boolean): VerificationGuard = lock synchronized {
Review Comment:
oops good call. We can change it. Let me do a general code search
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "artemlivshits (via GitHub)" <gi...@apache.org>.
artemlivshits commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936469374
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -569,20 +569,21 @@ class UnifiedLog(@volatile var logStartOffset: Long,
* Maybe create and return the VerificationGuard for the given producer ID if the transaction is not yet ongoing.
* Creation starts the verification process. Otherwise return the sentinel VerificationGuard.
*/
- def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short): VerificationGuard = lock synchronized {
+ def maybeStartTransactionVerification(producerId: Long, sequence: Int, epoch: Short, isTV2: Boolean): VerificationGuard = lock synchronized {
Review Comment:
I guess, the next question is whether we want to keep the name `isTV2` or change it to `supportsEpochBump` :-).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18654: Transaction Version 2 performance regression due to early return [kafka]
Posted by "artemlivshits (via GitHub)" <gi...@apache.org>.
artemlivshits commented on code in PR #18720:
URL: https://github.com/apache/kafka/pull/18720#discussion_r1936311685
##########
core/src/main/scala/kafka/server/AddPartitionsToTxnManager.scala:
##########
@@ -67,10 +67,14 @@ object AddPartitionsToTxnManager {
* genericErrorSupported: This maps to the case when the clients are updated to handle the TransactionAbortableException
* addPartition: This allows the partition to be added to the transactions inflight with the Produce and TxnOffsetCommit requests. Plus the behaviors in genericErrorSupported.
*/
-sealed trait TransactionSupportedOperation
+sealed trait TransactionSupportedOperation {
+ val isTV2 = false;
Review Comment:
Nit: do we need a semicolon here?
##########
core/src/main/scala/kafka/server/AddPartitionsToTxnManager.scala:
##########
@@ -67,10 +67,14 @@ object AddPartitionsToTxnManager {
* genericErrorSupported: This maps to the case when the clients are updated to handle the TransactionAbortableException
* addPartition: This allows the partition to be added to the transactions inflight with the Produce and TxnOffsetCommit requests. Plus the behaviors in genericErrorSupported.
*/
-sealed trait TransactionSupportedOperation
+sealed trait TransactionSupportedOperation {
+ val isTV2 = false;
Review Comment:
Should we name it `atLeastTV2` (if we have more protocol bumps they will inherit this override), or maybe `supportsEpochBump` to be consistent with TC naming (which tries to express the same thing).
##########
core/src/main/scala/kafka/server/ReplicaManager.scala:
##########
@@ -950,7 +958,7 @@ class ReplicaManager(val config: KafkaConfig,
): Unit = {
// Skip verification if the request is not transactional or transaction verification is disabled.
if (transactionalId == null ||
- !config.transactionLogConfig.transactionPartitionVerificationEnable
+ (!config.transactionLogConfig.transactionPartitionVerificationEnable && transactionSupportedOperation != addPartition)
Review Comment:
Should we use `transactionSupportedOperation.isTV2` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org